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
