libcpp: Fix ICE with -Wtraditional preprocessing [PR101638]

Hi!

The following testcase ICEs in cpp_sys_macro_p, because cpp_sys_macro_p
is called for a builtin macro which doesn't use node->value.macro union
member but a different one and so dereferencing it ICEs.
As the testcase is distilled from contemporary glibc headers, it means
basically -Wtraditional now ICEs on almost everything.

The fix can be either the patch below, return false for builtin macros,
or we could instead return true for builtin macros and adjust the
function comment, or the fix could be also (untested):

	Jakub

On 8/12/21 3:57 AM, Jakub Jelinek wrote:
> Hi!
> 
> The following testcase ICEs in cpp_sys_macro_p, because cpp_sys_macro_p
> is called for a builtin macro which doesn't use node->value.macro union
> member but a different one and so dereferencing it ICEs.
> As the testcase is distilled from contemporary glibc headers, it means
> basically -Wtraditional now ICEs on almost everything.
> 
> The fix can be either the patch below, return false for builtin macros,
> or we could instead return true for builtin macros and adjust the
> function comment, or the fix could be also (untested):
> --- libcpp/expr.c	2021-05-07 10:34:46.345122608 +0200
> +++ libcpp/expr.c	2021-08-12 09:54:01.837556365 +0200
> @@ -783,13 +783,13 @@ cpp_classify_number (cpp_reader *pfile,
>   
>         /* Traditional C only accepted the 'L' suffix.
>            Suppress warning about 'LL' with -Wno-long-long.  */
> -      if (CPP_WTRADITIONAL (pfile) && ! cpp_sys_macro_p (pfile))
> +      if (CPP_WTRADITIONAL (pfile))
>   	{
>   	  int u_or_i = (result & (CPP_N_UNSIGNED|CPP_N_IMAGINARY));
>   	  int large = (result & CPP_N_WIDTH) == CPP_N_LARGE
>   		       && CPP_OPTION (pfile, cpp_warn_long_long);
>   
> -	  if (u_or_i || large)
> +	  if ((u_or_i || large) && ! cpp_sys_macro_p (pfile))
>   	    cpp_warning_with_line (pfile, large ? CPP_W_LONG_LONG : CPP_W_TRADITIONAL,
>   				   virtual_location, 0,
>   				   "traditional C rejects the \"%.*s\" suffix",
> The builtin macros at least currently don't add any suffixes
> or numbers -Wtraditional would like to warn about.

So whether cpp_sys_macro_p returns true or false has no practical effect.

> For floating
> point suffixes, -Wtraditional calls cpp_sys_macro_p only right
> away before emitting the warning, but in the above case the ICE
> is because cpp_sys_macro_p is called even if the number doesn't
> have any suffixes (that is I think always for builtin macros
> right now).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux.
> Ok for trunk, or do you prefer to return true for builtin
> macros from cpp_sys_macro_p instead

I think I'd prefer to return true, since builtin macros are also in the 
broader category of macros provided by the implementation.  OK with that 
change.

, and/or do you want the
> above cpp_classify_number change (which can be done either
> alone or in addition to some cpp_sys_macro_p change)?
> 
> 2021-08-12  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR preprocessor/101638
> 	* macro.c (cpp_sys_macro_p): Return false instead of
> 	crashing on builtin macros.
> 
> 	* gcc.dg/cpp/pr101638.c: New test.
> 
> --- libcpp/macro.c.jj	2021-07-20 17:03:08.036449892 +0200
> +++ libcpp/macro.c	2021-08-11 20:50:11.212662720 +0200
> @@ -3127,7 +3127,10 @@ cpp_sys_macro_p (cpp_reader *pfile)
>     else
>       node = pfile->context->c.macro;
>   
> -  return node && node->value.macro && node->value.macro->syshdr;
> +  return (node
> +	  && cpp_user_macro_p (node)
> +	  && node->value.macro
> +	  && node->value.macro->syshdr);
>   }
>   
>   /* Read each token in, until end of the current file.  Directives are
> --- gcc/testsuite/gcc.dg/cpp/pr101638.c.jj	2021-08-11 20:53:04.785289640 +0200
> +++ gcc/testsuite/gcc.dg/cpp/pr101638.c	2021-08-11 20:52:36.086682006 +0200
> @@ -0,0 +1,7 @@
> +/* PR preprocessor/101638 */
> +/* { dg-do preprocess } */
> +/* { dg-options "-Wtraditional" } */
> +
> +#define foo(attr) __has_attribute(attr)
> +#if foo(__deprecated__)
> +#endif
> 
> 	Jakub
>

On Thu, Aug 12, 2021 at 10:46:23AM -0400, Jason Merrill wrote:
> > --- libcpp/expr.c	2021-05-07 10:34:46.345122608 +0200
> > +++ libcpp/expr.c	2021-08-12 09:54:01.837556365 +0200
> > @@ -783,13 +783,13 @@ cpp_classify_number (cpp_reader *pfile,
> >         /* Traditional C only accepted the 'L' suffix.
> >            Suppress warning about 'LL' with -Wno-long-long.  */
> > -      if (CPP_WTRADITIONAL (pfile) && ! cpp_sys_macro_p (pfile))
> > +      if (CPP_WTRADITIONAL (pfile))
> >   	{
> >   	  int u_or_i = (result & (CPP_N_UNSIGNED|CPP_N_IMAGINARY));
> >   	  int large = (result & CPP_N_WIDTH) == CPP_N_LARGE
> >   		       && CPP_OPTION (pfile, cpp_warn_long_long);
> > -	  if (u_or_i || large)
> > +	  if ((u_or_i || large) && ! cpp_sys_macro_p (pfile))
> >   	    cpp_warning_with_line (pfile, large ? CPP_W_LONG_LONG : CPP_W_TRADITIONAL,
> >   				   virtual_location, 0,
> >   				   "traditional C rejects the \"%.*s\" suffix",
> > The builtin macros at least currently don't add any suffixes
> > or numbers -Wtraditional would like to warn about.
> 
> So whether cpp_sys_macro_p returns true or false has no practical effect.

Yes.

> > For floating
> > point suffixes, -Wtraditional calls cpp_sys_macro_p only right
> > away before emitting the warning, but in the above case the ICE
> > is because cpp_sys_macro_p is called even if the number doesn't
> > have any suffixes (that is I think always for builtin macros
> > right now).
> > 
> > Bootstrapped/regtested on x86_64-linux and i686-linux.
> > Ok for trunk, or do you prefer to return true for builtin
> > macros from cpp_sys_macro_p instead
> 
> I think I'd prefer to return true, since builtin macros are also in the
> broader category of macros provided by the implementation.  OK with that
> change.

Ok, will test that.  Thanks.

	Jakub