Hi,
Am I OK to push these patches given the testing went OK? I'm thinking
probably, but I don't want to overstep.
Thanks,
Andrew
Andrew Burgess <aburg...@redhat.com> writes:
> Jeff Law <jeffreya...@gmail.com> writes:
>
>> On 2/10/24 10:26 AM, Andrew Burgess wrote:
>>> GDB makes use of the libiberty function buildargv for splitting the
>>> inferior (program being debugged) argument string in the case where
>>> the inferior is not being started under a shell.
>>>
>>> I have recently been working to improve this area of GDB, and noticed
>>> some unexpected behaviour to the libiberty function buildargv, when
>>> the input is a string consisting only of white space.
>>>
>>> What I observe is that if the input to buildargv is a string
>>> containing only white space, then buildargv will return an argv list
>>> containing a single empty argument, e.g.:
>>>
>>> char **argv = buildargv (" ");
>>> assert (*argv[0] == '\0');
>>> assert (argv[1] == NULL);
>>>
>>> We get the same output from buildargv if the input is a single space,
>>> or multiple spaces. Other white space characters give the same
>>> results.
>>>
>>> This doesn't seem right to me, and in fact, there appears to be a work
>>> around for this issue in expandargv where we have this code:
>>>
>>> /* If the file is empty or contains only whitespace, buildargv would
>>> return a single empty argument. In this context we want no arguments,
>>> instead. */
>>> if (only_whitespace (buffer))
>>> {
>>> file_argv = (char **) xmalloc (sizeof (char *));
>>> file_argv[0] = NULL;
>>> }
>>> else
>>> /* Parse the string. */
>>> file_argv = buildargv (buffer);
>>>
>>> I think that the correct behaviour in this situation is to return an
>>> empty argv array, e.g.:
>>>
>>> char **argv = buildargv (" ");
>>> assert (argv[0] == NULL);
>>>
>>> And it turns out that this is a trivial change to buildargv. The diff
>>> does look big, but this is because I've re-indented a block. Check
>>> with 'git diff -b' to see the minimal changes. I've also removed the
>>> work around from expandargv.
>>>
>>> When testing this sort of thing I normally write the tests first, and
>>> then fix the code. In this case test-expandargv.c has sort-of been
>>> used as a mechanism for testing the buildargv function (expandargv
>>> does call buildargv most of the time), however, for this particular
>>> issue the work around in expandargv (mentioned above) masked the
>>> buildargv bug.
>>>
>>> I did consider adding a new test-buildargv.c file, however, this would
>>> have basically been a copy & paste of test-expandargv.c (with some
>>> minor changes to call buildargv). This would be fine now, but feels
>>> like we would eventually end up with one file not being updated as
>>> much as the other, and so test coverage would suffer.
>>>
>>> Instead, I have added some explicit buildargv testing to the
>>> test-expandargv.c file, this reuses the test input that is already
>>> defined for expandargv.
>>>
>>> Of course, once I removed the work around from expandargv then we now
>>> do always call buildargv from expandargv, and so the bug I'm fixing
>>> would impact both expandargv and buildargv, so maybe the new testing
>>> is redundant? I tend to think more testing is always better, so I've
>>> left it in for now.
>> So just an FYI. Sometimes folks include the -b diffs as well for these
>> scenarios. THe problem with doing so (as I recently stumbled over
>> myself) is the bots which monitor the list and apply patches get quite
>> confused by that practice. Anyway, just something to be aware of.
>>
>> As for testing, I tend to agree, more is better unless we're highly
>> confident its redundant. So I'll go with your judgment on
>> redundant-ness of the test.
>>
>> As with the prior patch, you'll need to run it through the usual
>> bootstrap/regression cycle and cobble together a ChangeLog.
>>
>> OK once those things are taken care of.
>
> Jeff,
>
> Thanks for looking these patches over.
>
> For testing, using current(ish) gcc HEAD, on x86-64 GNU/Linux, I:
>
> ../src/configure --prefix=$(cd .. && pwd)/install
> make
> make check
>
> I did this with / without my patch and then:
>
> find . -name "*.sum"
> ... compare all .sum files ...
>
> There was no change in any of the .sum files.
>
> 1. Am I correct that this will have run the bootstrap test by default?
>
> 2. Is there any other testing I should be doing?
>
> 3. If not, am I OK to apply both patches in this series?
>
> Thanks,
> Andrew
