Re: [Crash-utility] [PATCH] Fix C99 compatibility issues in embedded copy of GDB

On 2023/03/02 16:30, HAGIO KAZUHITO(萩尾 一仁) wrote: > On 2023/03/02 14:34, lijiang wrote: >> >> Isn't this "int" needed? >> > >> > It is, but this part is not actually used (the file is more of a macro >> > library for bash, I guess, and the use in readline is merely an >> > afterthought). >> >> I also have similar concerns. Some upstream changes are not included in this patch, and >> these functions are in the current gdb-10.2, for example: > > Yes, they might not need to be fixed actually, so my intention is to fix > the minimum parts that Florian's test detected at this time. If we > cannot build crash with a newer gcc, we will fix them or rebase gdb > depending on the situation.. > >> In addition, for the readline part, in fact some early crash distributions used the >> system readline library, and enabled the feature with the option "--with-system-readline" >> in the crash tool. >> >> The advantages are that there are few changes in crash gdb-10.2.patch and can use new >> features in the readline library, which can be easy to update in the system. >> >> The disadvantage is that crash may not be compatible with the system readline library and >> need depend on the system readline library. >> >> The current crash-utility is using the embedded readline lib in gdb, maybe we also have to >> face the future changes. This may come with a trade-off. > > Yes, agree. > > If the upstream crash enables the --with-system-readline, we have to > maintain the compatibility for newer readlines. But we (at least I) are > not very familiar with the gdb to maintain, and rarely rebase it. Given > these, maybe the embedded readline is not so bad, although there will be > some cases like this time.. >

>> > The problem is that the upstream patch does not really apply to the GDB >> > 10.2 sources. None of this work is really forward-looking, given that >> > crash will eventually have to import a newer GDB version. >> >> True, but there is no plan to update the embedded GDB for now and it >> would be better to sync with the upstream code just in case, so I've >> updated some hunks like so. Does the attached patch work for you? >> >> Lianbo, I've changed the diff headers as you said, could you check >> and test this? >> >> With the attached patch, the test passed. But I still suggest some minor modifications: >> [1] add related upstream commits(/gnulib/GDB and glibc/) to patch log > > Ah forgot it, thanks. I'll add, is this ok? > > Related GDB commits: > 0075c53724f7 Impport libiberty commit: 885b6660c17f from gcc mainline. > b4f26d541aa7 Import GNU Readline 8.1 > 9c9d63b15ad5 gnulib: update to 776af40e0 > 3f3328b816ee Use startswith more for strncmp function calls. sorry, still missed: Related glibc commit: 2337e04e21ba cdefs: Limit definition of fortification macros >> [2] no need to add new files at the end of "tar xvzmf gdb-10.2.tar.gz" this time > > Yes, I found that we can restore the patched gdb files with this before > checking out the master (unpatched) branch, so kept it.. but if we > change to do so, it should be another patch. > > So will remove the tar hunk. >