Re: [PATCH] fixincludes: Update darwin_flt_eval_method for macOS 14

[Bruce Korb via Gcc-patches]

Looks reasonable to me!

On 8/16/23 12:20, Rainer Orth wrote:

On macOS 14, a guard in  changed:

-- MacOSX13.3.sdk/usr/include/math.h2023-04-19 01:54:44
+++ MacOSX14.0.sdk/usr/include/math.h   2023-08-01 08:42:43
@@ -22,0 +23 @@
+
@@ -43 +44 @@
-#if __FLT_EVAL_METHOD__ == 0
+#if __FLT_EVAL_METHOD__ == 0 || __FLT_EVAL_METHOD__ == -1
@@ -49 +50 @@
-#elif __FLT_EVAL_METHOD__ == 2 || __FLT_EVAL_METHOD__ == -1
+#elif __FLT_EVAL_METHOD__ == 2

Therefore the darwin_flt_eval_method fixincludes fix doesn't match any
longer, leading to a large number of testsuite failures like

/private/var/gcc/regression/master/14-gcc/build/gcc/include-fixed/math.h:69:5: error: 
#error "Unsupported value of __FLT_EVAL_METHOD__."

where __FLT_EVAL_METHOD__ = 16.

This patch adjusts the fix to allow for both forms.

Tested with make check in fixincludes on x86_64-apple-darwin23.0.0 and
verifying that  has indeed been fixed as expected.

Ok for trunk?

Rainer

Vim swap files not ignored

[Bruce Korb via Gcc-patches]

Hi,
I don't have the keys for write access anymore. This ought to be 
applied. Odd that it never has been. :)diff --git a/.gitignore b/.gitignore
index 14ee0325176..021a8c74185 100644
--- a/.gitignore
+++ b/.gitignore
@@ -6,6 +6,7 @@
 *~
 .#*
 *#
+.*.swp
 
 *.flt
 *.gmo

Re: [PATCH] fixincludes: simplify handling for access() failure [PR21283, PR80047]

[Bruce Korb via Gcc-patches]

Perfect.

On 11/12/21 1:58 PM, Xi Ruoyao wrote:

diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c
index 6dba2f6e830..ee57fbf61b4 100644
--- a/fixincludes/fixincl.c
+++ b/fixincludes/fixincl.c
@@ -1352,11 +1352,10 @@ process (void)
  
if (access (pz_curr_file, R_OK) != 0)

  {
-  int erno = errno;
-  fprintf (stderr, "Cannot access %s from %s\n\terror %d (%s)\n",
-   pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN),
-   erno, xstrerror (erno));
-  return;
+  /* Some really strange error happened. */
+  fprintf (stderr, "Cannot access %s: %s\n", pz_curr_file,
+  xstrerror (errno));
+  abort();
  }
  
pz_curr_data = load_file (pz_curr_file);

Re: [PATCH] fixincludes: fix portability issues about getcwd() [PR21283, PR80047]

[Bruce Korb via Gcc-patches]

If you are going to be excruciatingly, painfully correct, free() is 
going to be unhappy about freeing a static string in the event getcwd() 
fails for some inexplicable reason. I'd replace the free() + return with 
a call to exit. Maybe even:


   if (VERY_UNLIKELY (access (pz_curr_file, R_OK) != 0)) abort()

On 11/11/21 8:33 AM, Xi Ruoyao wrote:

---
  fixincludes/fixincl.c | 13 +++--
  1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fixincludes/fixincl.c b/fixincludes/fixincl.c
index 6dba2f6e830..1580c67efec 100644
--- a/fixincludes/fixincl.c
+++ b/fixincludes/fixincl.c
@@ -1353,9 +1353,18 @@ process (void)
if (access (pz_curr_file, R_OK) != 0)
  {
int erno = errno;
+  char *buf = NULL;
+  const char *cwd = NULL;
+  for (size_t size = 256; !cwd; size += size)
+   {
+ buf = xrealloc (buf, size);
+ cwd = getcwd (buf, size);
+ if (!cwd && errno != ERANGE)
+   cwd = "the working directory";
+   }
fprintf (stderr, "Cannot access %s from %s\n\terror %d (%s)\n",
-   pz_curr_file, getcwd ((char *) NULL, MAXPATHLEN),
-   erno, xstrerror (erno));
+  pz_curr_file, cwd, erno, xstrerror (erno));
+ free (buf); return;
  }
  

Re: [PATCH] fixincludes: don't assume getcwd() can handle NULL argument

[Bruce Korb via Gcc-patches]

On 11/10/21 4:22 AM, Xi Ruoyao wrote:

Isn't this warning actually a glibc bug
?

However we can't assume the libc we are using is Glibc.  Even if the
libc supports getcwd() with NULL argument, we are still leaking memory.
You could also free the memory by calling exit(2). Something is pretty 
wrong if fixincludes cannot access a file it was told to process. So, 
technically, you're right. Practically, no difference. But it's fine by 
me to make the change. It does avoid a bug in a certain version of a 
certain library.

Re: [PATCH v3] fixinc: don't "fix" machine names in __has_include(...) [PR91085]

[Bruce Korb via Gcc-patches]

On 6/28/21 10:26 PM, Xi Ruoyao wrote:

v3:
   use memmem/memchr instead of trivial loops
   split most of the logic into a static function
   avoid hardcoded magic number
   adjust test

Looks good to me. :)

Re: [PATCH v2] fixinc: don't "fix" machine names in __has_include(...) [PR91085]

[Bruce Korb via Gcc-patches]

Hi Xi,

On 6/27/21 11:07 PM, Xi Ruoyao wrote:

diff --git a/fixincludes/fixfixes.c b/fixincludes/fixfixes.c
index 5b23a8b640d..147cba716c7 100644
--- a/fixincludes/fixfixes.c
+++ b/fixincludes/fixfixes.c
@@ -524,7 +524,7 @@ FIX_PROC_HEAD( machine_name_fix )
/* If the 'name_pat' matches in between base and limit, we have
   a bogon.  It is not worth the hassle of excluding comments
   because comments on #if/#ifdef lines are rare, and strings on
- such lines are illegal.
+ such lines are only legal in a "__has_include" directive.
  
   REG_NOTBOL means 'base' is not at the beginning of a line, which

   shouldn't matter since the name_re has no ^ anchor, but let's
@@ -544,6 +544,31 @@ FIX_PROC_HEAD( machine_name_fix )
  break;
  
p = base + match[0].rm_so;


This function is already 90 lines long. This would be better in a function.


+
+  /* Check if the match is in __has_include(...) (PR 91085). */
+  for (q = base; q < p; q++)
+if (!strncmp (q, "__has_include", 13))
+  {
+r = q + 13;
+while (r < p && ISSPACE (*r))
+  r++;
+
+/* "__has_include" may appear as "defined(__has_include)",
+   search for the next appearance then.  */
+if (*r != '(')
+  continue;
+
+/* To avoid too much complexity, just hope there is never a
+   ')' in a header name.  */
+while (r < limit && *r != ')')
+  r++;


strchr()? I'd use strchr() to find the start of "__has_include" as well. 
A character-by-character search is more obtuse and any CPU cycle savings 
are pretty marginal. Also:


char const has_inc[] = "__has_include"; int const has_inc_len = 
sizeof(has_inc) - 1;


It makes what's going on more plain by eliminating a magic number (13).


+if (r >= base + match[0].rm_eo)
+  {
+base = r;
+goto again;
+  }
+  }
+
base += match[0].rm_eo;
  
/* One more test: if on the same line we have the same string

diff --git a/fixincludes/inclhack.def b/fixincludes/inclhack.def
index 3a4cfe06542..31389396af6 100644
--- a/fixincludes/inclhack.def
+++ b/fixincludes/inclhack.def
@@ -3151,7 +3151,8 @@ fix = {
  c_fix = machine_name;
  
  test_text = "/* MACH_DIFF: */\n"

-"#if defined( i386 ) || defined( sparc ) || defined( vax )"
+"#if defined( i386 ) || defined( sparc ) || defined( vax ) || "
+"defined( linux ) || __has_include (  ) || defined ( linux )"
No need for a redundant "defined(linux)" test. If you want to test 
superfluous spaces around the parentheses, just do it for one of the 
machine types.

  "\n/* no uniform test, so be careful  :-) */";
  };
  
diff --git a/fixincludes/tests/base/testing.h b/fixincludes/tests/base/testing.h

index cf95321fb86..00e8dde003e 100644
--- a/fixincludes/tests/base/testing.h
+++ b/fixincludes/tests/base/testing.h
@@ -64,7 +64,7 @@ BSD43__IOWR('T', 1) /* Some are multi-line */
  
  #if defined( MACHINE_NAME_CHECK )

  /* MACH_DIFF: */
-#if defined( i386 ) || defined( sparc ) || defined( vax )
+#if defined( i386 ) || defined( sparc ) || defined( vax ) || defined( linux ) || 
__has_include (  ) || defined ( linux )
  /* no uniform test, so be careful  :-) */
  #endif  /* MACHINE_NAME_CHECK */


Thanks for working on this.

Regards, Bruce

Re: Fw: Problems with compiling autogen with GCC8 or newer versions

[Bruce Korb via Gcc]

Hi,

On 1/10/21 3:56 PM, Martin Sebor wrote:

Sure.  I was confirming that based on the GCC dump there is no risk
of an overflow in the translation unit, and so there is no warning.


OK. :) I didn't understand the GCC dump. Despite having commit privs, 
I'm not actually a compiler guru.



It can /not/ overflow. Those compiler stats are not decipherable by me.


They indicate the minimum, likely, maximum, and unlikely maximum
number of bytes of output for each directive and the running totals
for the call (in parentheses).  The relevant lines are these:

  Directive 2 at offset 2: "%s"
    Result: 0, 255, 255, 255 (2, 257, 257, 257)

The result tells us that the length of the %s argument is between
0 and 255 bytes long.
It should be 1 to 255. 0 is actually impossible, but it would take crazy 
complicated sleuthing to figure it out, even though the "spn_*" 
functions should be inlined.


Since objsize (the size of the destination) is 520 there is no
buffer overflow.
The size of the destination is guaranteed to be between 263 and 518 
bytes. The "def_str" pointer will always point at least two bytes past 
the start of the 520 byte buffer.


The note in the forwarded message indicates that GCC computes
the destination size to be much smaller for some reason:

  note: 'sprintf' output between 4 and 259 bytes into a destination of 
size 255


I.e., it thinks it's just 255 bytes.  As I explained, such a small
size would trigger the warning by design.
Yep. If it can accurately figure out the minimum size remaining, that 
would be completely fine. "If."

  I can't really think of
a reason why GCC would compute a smaller size here (it looks far
from trivial).
If it can figure out that the minimum size is 263, that'd be great. If 
it can't, it needs to be quiet.

  We'd need to see the original poster's translation
unit and know the host and the target GCC was configured for.


OK. Not anything I can do. Thomas would have to send in his version of 
"gd.i".



Thank you!

Regards, Bruce

Re: Fw: Problems with compiling autogen with GCC8 or newer versions

[Bruce Korb via Gcc]

Hi Martin,

On 1/10/21 11:01 AM, Martin Sebor wrote:

On 1/8/21 12:38 PM, Bruce Korb via Gcc wrote:
This is the code that must be confusing to GCC. "def_str" points to 
the second character in the 520 byte buffer. "def_scan" points to a 
character that we already know we're going to copy into the 
destination, so the "spn" function doesn't look at it:


 {
 char * end = spn_ag_char_map_chars(def_scan + 1, 31);
 size_t len = end - def_scan;
 if (len >= 256)
 goto fail_return;
 memcpy(def_str, def_scan, len);
 def_str += len;
 *def_str = '\0';
 def_scan = end;
 }


In the function preamble, "def_str" points to the first character 
(character "0") of a 520 byte buffer. Before this fragment, "*def_str" 
is set to an apostrophe and the pointer advanced. After execution passes 
through this fragment, "def_str" is pointing to a NUL byte that can be 
as far as 257 bytes into the buffer (character "257"). That leaves 263 
more bytes. The "offending" sprintf is:


sprintf(def_str, "  %s'", name_bf);

GCC correctly determines that "name_bf" cannot contain more than 255 
bytes. Add 3 bytes of text and a NUL byte and the sprintf will be 
dropping *AT MOST* 259 characters into the buffer. The buffer is 4 bytes 
longer than necessary.




GCC 8 also doesn't warn but it does determine the size.  Here's
the output for the relevant directive (from the output of
-fdump-tree-printf-return-value in GCC versions prior to 10, or
-fdump-tree-strlen in GCC 10 and later).  objsize is the size
of the destination, or 520 bytes here (this is in contrast to
the 255 in the originally reported message). The Result numbers
are the minimum and maximum size of the output (between 0 and
255 characters).

Computing maximum subobject size for def_str_146:
getdefs.c:275: sprintf: objsize = 520, fmtstr = "  %s'"
  Directive 1 at offset 0: "  ", length = 2
    Result: 2, 2, 2, 2 (2, 2, 2, 2)
  Directive 2 at offset 2: "%s"
    Result: 0, 255, 255, 255 (2, 257, 257, 257)
  Directive 3 at offset 4: "'", length = 1
    Result: 1, 1, 1, 1 (3, 258, 258, 258)
  Directive 4 at offset 5: "", length = 1

Besides provable overflow, it's worth noting that -Wformat-overflow

It can /not/ overflow. Those compiler stats are not decipherable by me.

also diagnoses a subset of cases where it can't prove that overflow
cannot happen.  One common case is:

  extern char a[8], b[8];
  sprintf (a, "a=%s", b);
My example would have the "a" array sized at 16 bytes and "b" provably 
not containing more than 7 characters (plus a NUL). There would be no 
overflow.

... The solution is to either use precision
to constrain the amount of output or in GCC 10 and later to assert
that b's length is less than 7.
See the fragment below to see where the characters in name_bf can 
*/NOT/* be more than 255. There is no need for either a precision 
constraint or an assertion, based on that code fragment.


So if in the autogen file def_str is ever less than 258 bytes/[259 -- 
NUL byte, too]/

I'd expect the warning to trigger with a message close to
the original.


It can not be less than 263. For the sake of those not reading the code, 
here is the fragment that fills in "name_bf[256]":



    {
    char * end = spn_ag_char_map_chars(def_scan + 1, 26);
    size_t len = end - def_scan;
    if (len >= 256)
    goto fail_return;
    memcpy(name_bf, def_scan, len);
    name_bf[len] = '\0';
    def_scan = end;
    }

Re: Fw: Problems with compiling autogen with GCC8 or newer versions

[Bruce Korb via Gcc]

Hi,

You are supposed to be able to post once you've subscribed.

Also, GCC's code analysis is wrong. "name_bf" contains *NO MORE* than 
MAXNAMELEN characters. That is provable.


"def_str" points into a buffer of size ((MAXNAMELEN * 2) + 8) and at an 
offset maximum of MAXNAMELEN+1 (also provable), meaning that at a 
minimum there are MAXNAMELEN+6 bytes left in the buffer.


That objected-to sprintf can add a maximum of MAXNAMELEN + 4 to where 
"def_str" points.


GCC is wrong. It is unable to figure out how far into the buffer 
"def_str" can point.


On 1/8/21 2:26 AM, Oppe, Thomas C ERDC-RDE-ITL-MS Contractor wrote:

Dear Sir:

I would like to post the following message to the mailing list 
"autogen-us...@lists.sourceforge.net".  Could you please add me to this list?

I am an HPC user at ERDC DSRC in Vicksburg, MS.  One of my projects is building GCC 
snapshots and releases using various software prerequisite packages necessary in the 
"make check" phase.  One of these packages is autogen-5.18.16.

Thank you for your consideration.

Tom Oppe


-
Thomas C. Oppe
HPCMP Benchmarking Team
HITS Team SAIC
thomas.c.o...@erdc.dren.mil
Work:  (601) 634-2797
Cell:(601) 642-6391
-


From: Oppe, Thomas C ERDC-RDE-ITL-MS Contractor
Sent: Friday, January 8, 2021 12:32 AM
To: autogen-us...@lists.sourceforge.net
Subject: Problems with compiling autogen with GCC8 or newer versions

Dear Sir:

When compiling autogen-5.18.16 with gcc8 or newer, I am getting format overflow errors 
like the following during the "make" step:

top_builddir=".." top_srcdir=".." VERBOSE="" /bin/bash "../build-aux/run-ag.sh" 
-MFstamp-opts -MTstamp-opts -MP ./opts.def
gcc -DHAVE_CONFIG_H -I. -I..  -I.. -I../autoopts  -g -O2 
-I/p/home/oppe/gcc/10.2.0/include -Wno-format-contains-nul -fno-strict-aliasing 
-Wall -Werror -Wcast-align -Wmissing-prototypes -Wpointer-arith -Wshadow 
-Wstrict-prototypes -Wwrite-strings -Wstrict-aliasing=3 -Wextra -Wno-cast-qual 
-g -O2 -I/p/home/oppe/gcc/10.2.0/include -Wno-format-contains-nul 
-fno-strict-aliasing -c -o gd.o gd.c
In file included from gd.c:11:
getdefs.c: In function 'buildDefinition':
getdefs.c:451:29: error: '%s' directive writing up to 255 bytes into a region 
of size 253 [-Werror=format-overflow=]
   451 | sprintf(def_str, "  %s'", name_bf);
   | ^~~~~
getdefs.c:451:9: note: 'sprintf' output between 4 and 259 bytes into a 
destination of size 255
   451 | sprintf(def_str, "  %s'", name_bf);
   | ^~
cc1: all warnings being treated as errors
make[2]: *** [gd.o] Error 1
make[2]: Leaving directory `/p/work1/oppe/autogen-5.18.16/getdefs'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/p/work1/oppe/autogen-5.18.16'
make: *** [all] Error 2

Do I just add "-Wno-error=format-overflow" to the compile options?  Do you have 
a fix?

Tom Oppe

-
Thomas C. Oppe
HPCMP Benchmarking Team
HITS Team SAIC
thomas.c.o...@erdc.dren.mil
Work:  (601) 634-2797
Cell:(601) 642-6391
-

Re: Why was it important to change "FALLTHROUGH" to "fall through"?

[Bruce Korb via Gcc]

On Tue, Sep 8, 2020 at 7:36 AM Jakub Jelinek  wrote:
> > The fall through comment was polluted with a colon that I hadn't noticed, 
> > as in:
> >
> > /* FALLTHROUGH: */
> >
> > and your fall through regex doesn't allow for that.
> > I'd add a colon to the space, tab and '!' that the regex accepts.
>
> I think it is a bad idea to change the regexps, it has been done that way
> for quite a while and many people could rely on what exactly is and is not
> handled.

That is your call. I've never used the colon myself, but my friend did
in this example. Unfortunately, I don't get to pick what compiler
options folks like to pick for building my code, so I cannot choose
the fallthrough level of 2.

Anyway, the error message ought to include enough information that
folks can fix it without having to resort to multiple Google searches
and reading discussions. (Just mention "// fall through" in the
message.)

> There is always the option to use attributes or builtins...

Not if you're writing code for multiple platforms. :(

Thank you. Regards, Bruce

Re: Why was it important to change "FALLTHROUGH" to "fall through"?

[Bruce Korb via Gcc]

On Tue, Sep 8, 2020 at 2:33 AM Jonathan Wakely  wrote:
> > Nope. I had /* FALLTHROUGH */ on the line before a blank line before
> > the case label. After Googling, I found an explicit reference that you
> > had to spell it: // fall through
> > I did that, and it worked. So I'm moving on, but still ...
>
> The canonical reference is
> https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wimplicit-fallthrough
> and it says FALLTHROUGH is fine (except with -Wimplicit-fallthrough=5
> which "doesn’t recognize any comments as fallthrough comments, only
> attributes disable the warning").

Hi,

Thank you. It turns out it was in someone else's code that I'd
incorporated into my project.
The fall through comment was polluted with a colon that I hadn't noticed, as in:

/* FALLTHROUGH: */

and your fall through regex doesn't allow for that.
I'd add a colon to the space, tab and '!' that the regex accepts.
Does your acceptance pattern accept these? It's hard for me to decipher.

getdefs/getdefs.c:/* FALLTHROUGH */ /* NOTREACHED */
agen5/defLex.c:/* FALLTHROUGH */ /* to Invalid input char */

I'd also recommend a modified error message that includes mention of
the approved comment.

Thank you.

Regards, Bruce

Re: Why was it important to change "FALLTHROUGH" to "fall through"?

[Bruce Korb via Gcc]

On Mon, Sep 7, 2020 at 3:45 PM Florian Weimer  wrote:
>
> * Bruce Korb via Gcc:
>
> > I don't write a lot of code anymore, but this sure seems like a
> > gratuitous irritation to me. I've been using
> >
> > // FALLTHRU and
> > // FALLTHROUGH
> >
> > for *DECADES*, so it's pretty incomprehensible why the compiler should
> > have to invalidate my code because it thinks a different coding
> > comment is better.
>
> It's not clear what you are talking about.
>
> Presumably you placed the comment before a closing brace, and not
> immediately before the subsequent case label?

Nope. I had /* FALLTHROUGH */ on the line before a blank line before
the case label. After Googling, I found an explicit reference that you
had to spell it: // fall through
I did that, and it worked. So I'm moving on, but still ...

Why was it important to change "FALLTHROUGH" to "fall through"?

[Bruce Korb via Gcc]

I don't write a lot of code anymore, but this sure seems like a
gratuitous irritation to me. I've been using

// FALLTHRU and
// FALLTHROUGH

for *DECADES*, so it's pretty incomprehensible why the compiler should
have to invalidate my code because it thinks a different coding
comment is better.