On Thu, Mar 2, 2023 at 3:42 PM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab@xxxxxxx> wrote:
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..
>
Thanks for sharing your thoughts, Kazu.
Let's leave it there for now, it might be good to deal with this in the future if needed.
>> > 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.
>
Thank you, Kazu.
This looks good to me with the above modifications. So: Ack.
Lianbo
-- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility Contribution Guidelines: https://github.com/crash-utility/crash/wiki
