<div dir="ltr"><div dir="ltr">On Tue, Feb 28, 2023 at 12:33 PM HAGIO KAZUHITO(萩尾 一仁) <<a href="mailto:k-hagio-ab@nec.com" target="_blank">k-hagio-ab@nec.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 2023/02/22 16:50, Florian Weimer wrote:<br>
> * HAGIO KAZUHITO(萩尾 一仁):<br>
> <br>
>>> +@@ -123,14 +142,70 @@<br>
>>> + #define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1)<br>
>>> + #define __bos0(ptr) __builtin_object_size (ptr, 0)<br>
>>> +<br>
>>> ++/* Use __builtin_dynamic_object_size at _FORTIFY_SOURCE=3 when available. */<br>
>>> ++#if __USE_FORTIFY_LEVEL == 3 && (__glibc_clang_prereq (9, 0) \<br>
>>> ++ || __GNUC_PREREQ (12, 0))<br>
>>> ++# define __glibc_objsize0(__o) __builtin_dynamic_object_size (__o, 0)<br>
>>> ++# define __glibc_objsize(__o) __builtin_dynamic_object_size (__o, 1)<br>
>>> ++#else<br>
>>> ++# define __glibc_objsize0(__o) __bos0 (__o)<br>
>>> ++# define __glibc_objsize(__o) __bos (__o)<br>
>>> ++#endif<br>
>>> ++<br>
>><br>
>>> ++#if __USE_FORTIFY_LEVEL > 0<br>
>><br>
>> I could not find this line in the latest GDB source and the related<br>
>> patches. What is this for?<br>
> <br>
> We need to sync this again with gnulib/GDB. It was added here, in<br>
> glibc, which is the primary source of this file:<br>
<br>
I see, thanks.<br>
<br>
> <br>
> commit 2337e04e21ba6040926ec871e403533f77043c40<br>
> Author: Siddhesh Poyarekar <<a href="mailto:siddhesh@sourceware.org" target="_blank">siddhesh@sourceware.org</a>><br>
> Date: Thu Feb 2 07:49:02 2023 -0500<br>
> <br>
> cdefs: Limit definition of fortification macros<br>
> <br>
> Define the __glibc_fortify and other macros only when __FORTIFY_LEVEL ><br>
> 0. This has the effect of not defining these macros on older C90<br>
> compilers that do not have support for variable length argument lists.<br>
> <br>
> Also trim off the trailing backslashes from the definition of<br>
> __glibc_fortify and __glibc_fortify_n macros.<br>
> <br>
> Signed-off-by: Siddhesh Poyarekar <<a href="mailto:siddhesh@sourceware.org" target="_blank">siddhesh@sourceware.org</a>><br>
> Reviewed-by: Florian Weimer <<a href="mailto:fweimer@redhat.com" target="_blank">fweimer@redhat.com</a>><br>
> <br>
> I should have made a note that this is coming from the ultimate upstream<br>
> sources.<br>
> <br>
> I raised the issue of <sys/cdefs.h> syncing here:<br>
> <br>
> Updating <sys/cdefs.h> in glibc and gnulib<br>
> <<a href="https://sourceware.org/pipermail/libc-alpha/2023-February/145758.html" rel="noreferrer" target="_blank">https://sourceware.org/pipermail/libc-alpha/2023-February/145758.html</a>><br>
> <br>
>>> +@@ -134,6 +136,7 @@ typedef int (*_bashfunc)(const char *, ...);<br>
>>> + #else<br>
>>> + typedef int (*_bashfunc)();<br>
>>> + #endif<br>
>>> ++#include <stdlib.h><br>
>>> + main()<br>
>><br>
>> The GDB patch b4f26d541aa7 ("Import GNU Readline 8.1") has the<br>
>> following:<br>
>><br>
>> @@ -134,6 +138,8 @@ typedef int (*_bashfunc)(const char *, ...);<br>
>> #else<br>
>> typedef int (*_bashfunc)();<br>
>> #endif<br>
>> +#include <stdlib.h><br>
>> +int<br>
>> main()<br>
>> {<br>
>> _bashfunc pf;<br>
>><br>
>> Isn't this "int" needed?<br>
> <br>
> It is, but this part is not actually used (the file is more of a macro<br>
> library for bash, I guess, and the use in readline is merely an<br>
> afterthought).<br>
></blockquote><div><br></div><div>I also have similar concerns. Some upstream changes are not included in this patch, and</div><div>these functions are in the current gdb-10.2, for example:</div><div>...</div><div>--- a/readline/readline/aclocal.m4<br>+++ b/readline/readline/aclocal.m4<br><br>@@ -321,6 +334,9 @@ AC_CACHE_VAL(bash_cv_opendir_not_robust,<br> #ifdef HAVE_UNISTD_H<br> # include <unistd.h><br> #endif /* HAVE_UNISTD_H */<br>+#ifdef HAVE_SYS_STAT_H<br>+#include <sys/stat.h><br>+#endif<br> #if defined(HAVE_DIRENT_H)<br> # include <dirent.h><br> #else<br>...<br>@@ -300,7 +312,8 @@ AC_DEFUN(BASH_FUNC_STRSIGNAL,<br> [AC_MSG_CHECKING([for the existence of strsignal])<br> AC_CACHE_VAL(bash_cv_have_strsignal,<br> [AC_TRY_LINK([#include <sys/types.h><br>-#include <signal.h>],<br>+#include <signal.h><br>+#include <string.h>],<br> [char *s = (char *)strsignal(2);],<br> bash_cv_have_strsignal=yes, bash_cv_have_strsignal=no)])<br> AC_MSG_RESULT($bash_cv_have_strsignal)<br>...<br>@@ -684,6 +706,11 @@ AC_DEFUN(BASH_FUNC_ULIMIT_MAXFDS,<br> [AC_MSG_CHECKING(whether ulimit can substitute for getdtablesize)<br> AC_CACHE_VAL(bash_cv_ulimit_maxfds,<br> [AC_TRY_RUN([<br>+#include <stdlib.h><br>+#ifdef HAVE_ULIMIT_H<br>+#include <ulimit.h><br>+#endif<br>+int<br></div><div><br></div><div><br></div><div>In addition, for the readline part, in fact some early crash distributions used the</div><div>system readline library, and enabled the feature with the option "--with-system-readline"</div><div>in the crash tool.</div><div><br></div><div>The advantages are that there are few changes in crash gdb-10.2.patch and can use new</div><div>features in the readline library, which can be easy to update in the system.</div><div><br></div><div>The disadvantage is that crash may not be compatible with the system readline library and</div><div>need depend on the system readline library.</div><div><br></div><div>The current crash-utility is using the embedded readline lib in gdb, maybe we also have to</div><div>face the future changes. This may come with a trade-off. </div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br>
> The problem is that the upstream patch does not really apply to the GDB<br>
> 10.2 sources. None of this work is really forward-looking, given that<br>
> crash will eventually have to import a newer GDB version.<br>
<br>
True, but there is no plan to update the embedded GDB for now and it<br>
would be better to sync with the upstream code just in case, so I've<br>
updated some hunks like so. Does the attached patch work for you? <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Lianbo, I've changed the diff headers as you said, could you check<br>
and test this?<br>
<br></blockquote><div> </div><div>With the attached patch, the test passed. But I still suggest some minor modifications:</div><div>[1] add related upstream commits(<i style="color:rgb(0,0,0);white-space:pre-wrap">gnulib/GDB and glibc</i>) to patch log</div><div>[2] no need to add new files at the end of "tar xvzmf gdb-10.2.tar.gz" this time</div><div> </div><div>$ grep ^+++ gdb-10.2.patch | sort | uniq -c<br> 1 +++ gdb-10.2/bfd/elf-bfd.h<br> 1 +++ gdb-10.2/gdb/ada-lang.c<br> 1 +++ gdb-10.2/gdb/cli/cli-cmds.c<br> 1 +++ gdb-10.2/gdb/completer.c<br> 1 +++ gdb-10.2/gdb/c-typeprint.c<br> 1 +++ gdb-10.2/gdb/defs.h<br> 1 +++ gdb-10.2/gdb/dwarf2/read.c<br> 1 +++ gdb-10.2/gdb/gdbtypes.c<br> 1 +++ gdb-10.2/gdb/main.c<br> 2 +++ gdb-10.2/gdb/Makefile.in<br> 1 +++ gdb-10.2/gdb/objfiles.h<br> 2 +++ gdb-10.2/gdb/printcmd.c<br> 1 +++ gdb-10.2/gdb/psymtab.c<br> 2 +++ gdb-10.2/gdb/symfile.c<br> 3 +++ gdb-10.2/gdb/symtab.c<br> 1 +++ gdb-10.2/gdb/ui-file.h<br> 1 +++ gdb-10.2/gdb/xml-syscall.c<br> 1 +++ gdb-10.2/gnulib/import/cdefs.h<br> 1 +++ gdb-10.2/libiberty/aclocal.m4<br> 1 +++ gdb-10.2/libiberty/configure<br> 1 +++ gdb-10.2/libiberty/Makefile.in<br> 1 +++ gdb-10.2/Makefile.in<br> 1 +++ gdb-10.2/opcodes/i386-dis.c<br> 1 +++ gdb-10.2/readline/readline/aclocal.m4<br> 1 +++ gdb-10.2/readline/readline/configure<br> 1 +++ gdb-10.2/readline/readline/<a href="http://configure.ac" target="_blank">configure.ac</a><br> 1 +++ gdb-10.2/readline/readline/misc.c<br> 1 +++ gdb-10.2/readline/readline/readline.h<br> 1 +++ gdb-10.2/readline/readline/rltypedefs.h<br> 1 +++ gdb-10.2/readline/readline/util.c<br></div><div><br></div><div><br></div><div>Thanks.</div><div>Lianbo</div><div><br></div></div></div>