Re: [PATCHv2 2/2] libiberty/buildargv: handle input consisting of only white space

[Andrew Burgess <aburgess@redhat.com>]

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 <aburgess@redhat.com> writes:

> Jeff Law <jeffreyalaw@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