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.
I meant to say: I used to do this (diff -b), but I stopped as it was
causing more problems. Not just bots trying to apply patches, but
reviewers who want to download and apply patches to test.
I'm happy to send the reduced diff if it helps though.
Thanks,
Andrew
