Message ID | cd5986cd3374bda0deb6bd495d0dcec61dcaefa0.camel@mengyan1223.wang |
---|---|
State | New |
Headers | show |
Series | fixincludes: don't abort() on access failure [PR103306] | expand |
On 11/18/2021 4:01 AM, Xi Ruoyao via Gcc-patches wrote: > Some distro may ship dangling symlinks in include directories, triggers > the access failure. Skip it and continue to next header instead of > being to panic. > > Restore to old behavior before r12-5234 but without resurrecting the > problematic getcwd() call, by using the environment variable "INPUT" > exported by fixinc.sh. > > Tested on x86_64-linux-gnu, with a dangling symlink in /usr/include. > > fixincludes/ > > PR bootstrap/103306 > * fixincl.c (process): Don't call abort(). > --- > fixincludes/fixincl.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c > index a17b65866c3..81939ee5ffa 100644 > --- a/fixincludes/fixincl.c > +++ b/fixincludes/fixincl.c > @@ -1352,10 +1352,19 @@ process (void) > > if (access (pz_curr_file, R_OK) != 0) > { > - /* Some really strange error happened. */ > - fprintf (stderr, "Cannot access %s: %s\n", pz_curr_file, > + /* It may happens if for e. g. the distro ships some broken symlinks > + * in /usr/include. */ > + > + /* "INPUT" is exported in fixinc.sh, which is the pwd where fixincl > + * runs. It's used instead of getcwd to avoid allocating a buffer > + * with unknown length. */ Formatting nits. We don't use '*' at the start of comment lines. Drop the '*' like this /* blah blah blah more text. */ > + const char *cwd = getenv ("INPUT"); > + if (!cwd) > + cwd = "the working directory"; > + > + fprintf (stderr, "Cannot access %s from %s: %s\n", pz_curr_file, cwd, > xstrerror (errno)); > - abort (); > + return; > } If INPUT is always exported, why not just print it? ie, would CWD after actually be NULL? jeff
On Mon, 2021-11-22 at 17:37 -0700, Jeff Law wrote: > > > On 11/18/2021 4:01 AM, Xi Ruoyao via Gcc-patches wrote: > > Some distro may ship dangling symlinks in include directories, > > triggers > > the access failure. Skip it and continue to next header instead of > > being to panic. > > > > Restore to old behavior before r12-5234 but without resurrecting the > > problematic getcwd() call, by using the environment variable "INPUT" > > exported by fixinc.sh. > > > > Tested on x86_64-linux-gnu, with a dangling symlink in /usr/include. > > > > fixincludes/ > > > > PR bootstrap/103306 > > * fixincl.c (process): Don't call abort(). > > --- > > fixincludes/fixincl.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c > > index a17b65866c3..81939ee5ffa 100644 > > --- a/fixincludes/fixincl.c > > +++ b/fixincludes/fixincl.c > > @@ -1352,10 +1352,19 @@ process (void) > > > > if (access (pz_curr_file, R_OK) != 0) > > { > > - /* Some really strange error happened. */ > > - fprintf (stderr, "Cannot access %s: %s\n", pz_curr_file, > > + /* It may happens if for e. g. the distro ships some broken symlinks > > + * in /usr/include. */ > > + > > + /* "INPUT" is exported in fixinc.sh, which is the pwd where fixincl > > + * runs. It's used instead of getcwd to avoid allocating a buffer > > + * with unknown length. */ > Formatting nits. We don't use '*' at the start of comment lines. Drop > the '*' like this > > /* blah blah blah > more text. */ Strangely contrib/check_GNU_style.sh does not warn about this. > > > + const char *cwd = getenv ("INPUT"); > > + if (!cwd) > > + cwd = "the working directory"; > > + > > + fprintf (stderr, "Cannot access %s from %s: %s\n", pz_curr_file, cwd, > > xstrerror (errno)); > > - abort (); > > + return; > > } > If INPUT is always exported, why not just print it? ie, would CWD after > actually be NULL? INPUT is set by fixinc.sh. During GCC building process fixincl is always invoked by fixinc.sh. However someone may run fixincl executable directly for debugging.
On 11/23/2021 2:31 AM, Xi Ruoyao wrote: > On Mon, 2021-11-22 at 17:37 -0700, Jeff Law wrote: >> >> On 11/18/2021 4:01 AM, Xi Ruoyao via Gcc-patches wrote: >>> Some distro may ship dangling symlinks in include directories, >>> triggers >>> the access failure. Skip it and continue to next header instead of >>> being to panic. >>> >>> Restore to old behavior before r12-5234 but without resurrecting the >>> problematic getcwd() call, by using the environment variable "INPUT" >>> exported by fixinc.sh. >>> >>> Tested on x86_64-linux-gnu, with a dangling symlink in /usr/include. >>> >>> fixincludes/ >>> >>> PR bootstrap/103306 >>> * fixincl.c (process): Don't call abort(). >>> --- >>> fixincludes/fixincl.c | 15 ++++++++++++--- >>> 1 file changed, 12 insertions(+), 3 deletions(-) >>> >>> diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c >>> index a17b65866c3..81939ee5ffa 100644 >>> --- a/fixincludes/fixincl.c >>> +++ b/fixincludes/fixincl.c >>> @@ -1352,10 +1352,19 @@ process (void) >>> >>> if (access (pz_curr_file, R_OK) != 0) >>> { >>> - /* Some really strange error happened. */ >>> - fprintf (stderr, "Cannot access %s: %s\n", pz_curr_file, >>> + /* It may happens if for e. g. the distro ships some broken symlinks >>> + * in /usr/include. */ >>> + >>> + /* "INPUT" is exported in fixinc.sh, which is the pwd where fixincl >>> + * runs. It's used instead of getcwd to avoid allocating a buffer >>> + * with unknown length. */ >> Formatting nits. We don't use '*' at the start of comment lines. Drop >> the '*' like this >> >> /* blah blah blah >> more text. */ > Strangely contrib/check_GNU_style.sh does not warn about this. It should. Though in fairness, that checker is new relative to the overall live of the GCC project and obviously not 100% complete. Patches are always appreciated :-) > >>> + const char *cwd = getenv ("INPUT"); >>> + if (!cwd) >>> + cwd = "the working directory"; >>> + >>> + fprintf (stderr, "Cannot access %s from %s: %s\n", pz_curr_file, cwd, >>> xstrerror (errno)); >>> - abort (); >>> + return; >>> } >> If INPUT is always exported, why not just print it? ie, would CWD after >> actually be NULL? > INPUT is set by fixinc.sh. During GCC building process fixincl is > always invoked by fixinc.sh. However someone may run fixincl executable > directly for debugging. Good point. With the formatting nit fixed, this is fine for the trunk. Thanks, jeff
diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c index a17b65866c3..81939ee5ffa 100644 --- a/fixincludes/fixincl.c +++ b/fixincludes/fixincl.c @@ -1352,10 +1352,19 @@ process (void) if (access (pz_curr_file, R_OK) != 0) { - /* Some really strange error happened. */ - fprintf (stderr, "Cannot access %s: %s\n", pz_curr_file, + /* It may happens if for e. g. the distro ships some broken symlinks + * in /usr/include. */ + + /* "INPUT" is exported in fixinc.sh, which is the pwd where fixincl + * runs. It's used instead of getcwd to avoid allocating a buffer + * with unknown length. */ + const char *cwd = getenv ("INPUT"); + if (!cwd) + cwd = "the working directory"; + + fprintf (stderr, "Cannot access %s from %s: %s\n", pz_curr_file, cwd, xstrerror (errno)); - abort (); + return; } pz_curr_data = load_file (pz_curr_file);