After the commit:
commit 5e1d530da8 (gcc-buildargv)
Date: Sat Feb 10 11:22:13 2024 +0000
libiberty/buildargv: handle input consisting of only white space
The function only_whitespace (in argv.c) was no longer being called.
Lets delete it.
There should be no user visible changes after this commit.
2024-07-29 Andrew Burgess <aburgess@redhat.com>
libiberty/
* argv.c (only_whitespace): Delete.
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.
2024-07-16 Andrew Burgess <aburgess@redhat.com>
libiberty/
* argv.c (buildargv): Treat input of only whitespace as an empty
argument list.
(expandargv): Remove work around for intput that is only
whitespace.
* testsuite/test-expandargv.c: Add new tests 10, 11, and 12.
Extend testing to call buildargv in more cases.
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 have
tracked done some of the unexpected behaviour to the libiberty
function buildargv, and how it handles backslash escapes.
For reference, I've been mostly reading:
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
The issues that I would like to fix are:
1. Backslashes within single quotes should not be treated as an
escape, thus: '\a' should split to \a, retaining the backslash.
2. Backslashes within double quotes should only act as an escape if
they are immediately before one of the characters $ (dollar),
` (backtick), " (double quote), ` (backslash), or \n (newline). In
all other cases a backslash should not be treated as an escape
character. Thus: "\a" should split to \a, but "\$" should split to
$.
3. A backslash-newline sequence should be treated as a line
continuation, both the backslash and the newline should be removed.
I've updated libiberty and also added some tests. All the existing
libiberty tests continue to pass, but I'm not sure if there is more
testing that should be done, buildargv is used within lto-wraper.cc,
so maybe there's some testing folk can suggest that I run?
2024-07-16 Andrew Burgess <aburgess@redhat.com>
libiberty/
* argv.c (buildargv): Backslashes within single quotes are
literal, backslashes only escape POSIX defined special characters
within double quotes, and backslashed newlines should act as line
continuations.
* testsuite/test-expandargv.c: Add new tests 7, 8, and 9.
You are right, this is also a remnant of the old function design
that I completely missed. Here is the follow-up patch for that.
Thanks for pointing it out.
Costas
On Tue, 6 Jun 2023 at 04:12, Jeff Law <jeffreyalaw@gmail.com> wrote:
On 6/5/23 08:37, Costas Argyris via Gcc-patches wrote:
> writeargv can be simplified by getting rid of the error exit mode
> that was only relevant many years ago when the function used
> to open the file descriptor internally.
[ ... ]
Thanks. I've pushed this to the trunk.
You could (as a follow-up) simplify it even further. There's no need
for the status variable as far as I can tell. You could just have the
final return be "return 0;" instead of "return status;".
libiberty/
* argv.c (writeargv): Constant propagate "0" for "status",
simplifying the code slightly.
writeargv can be simplified by getting rid of the error exit mode
that was only relevant many years ago when the function used
to open the file descriptor internally.
0001-libiberty-writeargv-Simplify-function-error-mode.patch
From 1271552baee5561fa61652f4ca7673c9667e4f8f Mon Sep 17 00:00:00 2001
From: Costas Argyris <costas.argyris@gmail.com>
Date: Mon, 5 Jun 2023 15:02:06 +0100
Subject: [PATCH] libiberty: writeargv: Simplify function error mode.
The goto-based error mode was based on a previous version
of the function where it was responsible for opening the
file, so it had to close it upon any exit:
https://inbox.sourceware.org/gcc-patches/20070417200340.GM9017@sparrowhawk.codesourcery.com/
(thanks pinskia)
This is no longer the case though since now the function
takes the file descriptor as input, so the exit mode on
error can be just a simple return 1 statement.
libiberty/
* argv.c (writeargv): Simplify & remove gotos.
Signed-off-by: Costas Argyris <costas.argyris@gmail.com>
writeargv writes out empty arguments in a way that expandargv skips
them instead of preserving them. Fixed by writing out a pair of
quotes for them.
for libiberty/ChangeLog
* argv.c (writeargv): Output empty args as "".
Would be more useful if we could use "const char * const *", but there's
a long standing bug where gcc warns about incompatible pointers when you
try to pass in "char **". We can at least constify the array itself as
gcc will not warn in that case.
From-SVN: r232089
2009-10-08 Daniel Gutson <dgutson@codesourcery.com>
Daniel Jacobowitz <dan@codesourcery.com>
Pedro Alves <pedro@codesourcery.com>
libiberty/
* argv.c (consume_whitespace): New function.
(only_whitespace): New function.
(buildargv): Always use ISSPACE by calling consume_whitespace.
(expandargv): Skip empty files. Do not stop at the first empty
argument (calling only_whitespace)..
* testsuite/test-expandargv.c: (test_data): Test empty lines
and empty arguments.
(run_tests): Fix false positives due to shorter arguments.
Co-Authored-By: Daniel Jacobowitz <dan@codesourcery.com>
Co-Authored-By: Pedro Alves <pedro@codesourcery.com>
From-SVN: r152560
include/
2007-05-07 Nathan Froyd <froydnj@codesourcery.com>
* libiberty.h (writeargv): Declare.
libiberty/
2007-05-07 Nathan Froyd <froydnj@codesourcery.com>
* argv.c (writeargv): New function.
gcc/
2007-05-07 Nathan Froyd <froydnj@codesourcery.com>
* gcc.c (at_file_supplied): New variable.
(main): Set it if we expanded argv.
(do_spec_1): Pass an @-file to the linker if we were called with
an @-file argument and HAVE_GNU_LD.
* collect2.c (at_file_supplied): New variable.
(response_file): New variable.
(collect_exit): Unlink response_file if necessary.
(handler): Likewise.
(do_wait): Likewise.
(main): Set at_file_supplied if we expanded argv.
(collect_execute): Pass an @-file to subprocesses if we were called
with an @-file argument.
* configure.ac: Add define for HAVE_GNU_LD.
* configure: Regenerate.
* config.in: Regenerate.
From-SVN: r124532
libiberty/
2006-01-20 Carlos O'Donell <carlos@codesourcery.com>
* testsuite/Makefile.in: Add test-expandargv test.
* testsuite/test-expandargv.c: New test.
* argv.c (expandargv): Check for errors with ferror,
rather than just by looking at return value from fread.
From-SVN: r110047
* libiberty.h (expandargv): New function.
* argv.c (safe-ctype.h): Include it.
(ISBLANK): Remove.
(stdio.h): Include.
(buildargv): Use ISSPACE instead of ISBLANK.
(expandargv): New function.
From-SVN: r104664
* argv.c: Fix comments.
* calloc.c: Don't unnecessarily include "libiberty.h".
(bzero): Add prototype.
* floatformat.c: Include "ansidecl.h", rely on ANSI_PROTOTYPES.
* getcwd.c (getcwd): Use standard definition to avoid conflicts
with system headers.
* hashtab.c (htab_traverse): Delete unused variables.
* rename.c: Include "ansidecl.h".
(rename): Use standard definition to avoid conflicts with system
headers.
* strsignal.c: Rely on ANSI_PROTOTYPES.
* strstr.c: Check GNUC >= 2, not GNUC == 2.
* vfprintf.c: Include "ansidecl.h", rely on ANSI_PROTOTYPES.
* vprintf.c: Include "ansidecl.h" earlier, rely on
ANSI_PROTOTYPES.
* vsprintf.c: Include "ansidecl.h" earlier, rely on
ANSI_PROTOTYPES and possibly include <stdarg.h>.
* Makefile.in: Regenerate dependencies.
From-SVN: r65659
include:
* safe-ctype.h: New file.
libiberty:
* safe-ctype.c: New file.
* Makefile.in (CFILES): Add safe-ctype.c.
(REQUIRED_OFILES): Add safe-ctype.o.
* argv.c: Define ISBLANK and use it, not isspace.
* basename.c, cplus-dem.c, fnmatch.c, pexecute.c, strtod.c,
strtol.c, strtoul.c: Include safe-ctype.h, not ctype.h. Use
uppercase ctype macros. Don't test ISUPPER(c)/ISLOWER(c)
before calling TOLOWER(c)/TOUPPER(c).
gcc:
* Makefile.in (HOST_RTL): Add safe-ctype.o.
(safe-ctype.o): New rule.
* system.h: Include safe-ctype.h, not ctype.h. No need to
wrap ctype macros.
* cpphash.h: Zap IStable and related macros. Define is_* in
terms of safe-ctype.h macros.
* cppinit.c: Delete the IStable and all related code.
* tradcpp.c: Delete is_idchar, is_idstart, is_hor_space, and
is_space arrays. Delete initialize_char_syntax. Change all
references to the above arrays to use macros instead.
* tradcpp.h: Define is_idchar, is_idstart, is_space, and
is_nvspace in terms of safe_ctype.h's macros.
* tradcif.y: is_idchar, is_idstart are macros not arrays.
* config/i370/i370.c, config/winnt/dirent.c,
config/winnt/fixinc-nt.c, config/winnt/ld.c:
Use uppercase ctype macros. If we included ctype.h,
include safe-ctype.h instead.
* fixinc/fixfixes.c: Use uppercase ctype macros. Don't test
ISLOWER(c) before calling TOUPPER(c).
* fixinc/fixincl.c (extract_quoted_files): Simplify out some gunk.
* fixinc/gnu-regex.c: Include safe-ctype.h, not ctype.h. No need to
wrap ctype macros. Don't test ISUPPER(x) before calling TOLOWER(x).
gcc/ch:
* lex.c: Don't bother checking whether ISUPPER(c) before
calling TOLOWER(c). Don't bother checking whether isascii(c)
before testing ISSPACE(c); ISSPACE(c) includes '\n'.
gcc/f:
* Make-lang.in: Link f/fini with safe-ctype.o.
* bad.c: Don't test ISUPPER(c) || ISLOWER(c) before calling TOUPPER(c).
* com.c: Use TOUPPER, not ffesrc_toupper.
* fini.c: Don't test ISALPHA(c) before calling TOUPPER(c)/TOLOWER(c).
* intrin.c: Don't test IN_CTYPE_DOMAIN(c).
* src.c: Delete ffesrc_toupper_ and ffesrc_tolower_ and their
initializing code; use TOUPPER and TOLOWER instead of
ffesrc_toupper and ffesrc_tolower.
* src.h: Don't declare ffesrc_toupper_ or ffesrc_tolower_.
Don't define ffesrc_toupper or ffesrc_tolower.
gcc/java:
* jvgenmain.c: Use ISPRINT not isascii.
From-SVN: r38124
* argv.c (buildargv): Cast the result of alloca in assignment.
* choose-temp.c: Include stdlib.h.
* cplus-dem.c (demangle_arm_pt): Remove unused prototype.
(snarf_numeric_literal): Constify first parameter.
(code_for_qualifier): Avoid a gcc extension, make the parameter an
int, not a char.
(demangle_qualifier): Likewise.
(demangle_signature): Cast the argument of a ctype function to
unsigned char.
(arm_pt): Add parens around assignment used as truth value.
(demangle_arm_hp_template): Constify variable `args'.
(do_hpacc_template_const_value): Cast the argument of a ctype
function to unsigned char.
(do_hpacc_template_literal): Remove unused variable `i'.
(snarf_numeric_literal): Constify parameter `args'.
Cast the argument of a ctype function to unsigned char.
* floatformat.c (floatformat_to_double): Add explicit braces to
avoid ambiguous `else'.
* fnmatch.c (fnmatch): Change type of variables `c', `c1',
`cstart' and `cend' to unsigned char. Cast the argument of macro
`FOLD', which uses ctype functions, to unsigned char.
* objalloc.c (free): Add prototype.
From-SVN: r24392
Tue Oct 14 12:01:00 1997 Mark Mitchell <mmitchell@usa.net>
* cplus-dem.c (demangle_signature): Don't look for return types on
constructors. Handle member template constructors.
and update from devo.
From-SVN: r15901
