Bug 97264 - [11 Regression] -fpa-modref breaks va_arg on glibc
Summary: [11 Regression] -fpa-modref breaks va_arg on glibc
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: ipa (show other bugs)
Version: 11.0
: P3 normal
Target Milestone: 11.0
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-10-01 08:37 UTC by Sergei Trofimovich
Modified: 2020-10-16 12:09 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
uname.c (523 bytes, text/x-csrc)
2020-10-01 08:38 UTC, Sergei Trofimovich
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sergei Trofimovich 2020-10-01 08:37:28 UTC
Initially spotted the problem on glibc form master where fscanf() breaks in complex ways.

I tried to minimize fscanf() down to a single file. Looks like -fno-ipa-modref changes crashing to non-crashing program.

Full example:

// extracted from glibc's __vfscanf_internal()

void __isoc99_fscanf__ (const char *format, ...) __attribute__((noipa));
void __vfscanf_internal__ (const char *format, __builtin_va_list argptr)  __attribute__((noipa));

#define isd(c) ({ int __c = (c); __c >= '0' && __c <= '9'; })

static int
read_int (const unsigned char **pstr)
{
  // read first digit '1'
  int retval = **pstr - '0';

  // skip digits: executes 0 times and advances pointer once
  while (isd (*++(*pstr)))
      ;

  return retval;
}

void __vfscanf_internal__ (const char *f, __builtin_va_list argptr)
{
  /* assume input: "1<>\0" */
  while (*f != '\0')
    {
      const unsigned char ** pstr = (const unsigned char **) &f;
      // skip digit '1'
      read_int (pstr);

      /* Find the conversion specifier.  */
      f++; // skip '<'

      char * str = __builtin_va_arg(argptr, char *);

      f++; // skip ">"

      *str++ = '?';
    }
}

void __isoc99_fscanf__ (const char *format, ...)
{
  __builtin_va_list arg;

  __builtin_va_start (arg, format);
  __vfscanf_internal__ (format, arg);
  __builtin_va_end (arg);
}

int main (void)
{
    char key[10];
    __isoc99_fscanf__ ("1<>", key);
    return 0;
}

How to crash:

$ gcc-11.0.0 -O2 uname.c -o uname11 && ./uname11
Segmentation fault (core dumped)
$ gcc-11.0.0 -O2 uname.c -o uname11 -fno-ipa-modref && ./uname11
<ok>
Comment 1 Sergei Trofimovich 2020-10-01 08:38:12 UTC
Created attachment 49295 [details]
uname.c
Comment 2 Richard Biener 2020-10-01 09:41:08 UTC
a-t5.c.036t.fre1:ipa-modref: in __vfscanf_internal__/1, call to read_int/0 does not clobber f 2->2
a-t5.c.036t.fre1:ipa-modref: in __vfscanf_internal__/1, call to read_int/0 does not clobber f 2->2

I think this is 'const char *' vs. 'const unsigned char *' being TBAA
incompatible.  Changing read_int to take 'const char *pstr' fixes
things (and changing the type in __vfscanf_internal__ as well).

So this seems to be a latent bug in glibc unless somehow C makes those
pointers magically compatible wrt TBAA.  Joseph?
Comment 3 Sergei Trofimovich 2020-10-01 09:50:51 UTC
Oh, that makes sense.

> void __vfscanf_internal__ (const char *f, __builtin_va_list argptr)
> {
>   /* assume input: "1<>\0" */
>   while (*f != '\0')
>     {
>       const unsigned char ** pstr = (const unsigned char **) &f;

comes from https://sourceware.org/git/?p=glibc.git;a=blob;f=stdio-common/vfscanf-internal.c;h=95b46dcbeb55b1724b396f02a940f3047259b926;hb=HEAD#l489 :

"""
 274 int
 275 __vfscanf_internal (FILE *s, const char *format, va_list argptr,
 276                     unsigned int mode_flags)
...
 487       if (ISDIGIT ((UCHAR_T) *f))
 488         {
 489           argpos = read_int ((const UCHAR_T **) &f);
"""
Comment 4 rguenther@suse.de 2020-10-01 09:53:41 UTC
On Thu, 1 Oct 2020, slyfox at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97264
> 
> --- Comment #3 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
> Oh, that makes sense.
> 
> > void __vfscanf_internal__ (const char *f, __builtin_va_list argptr)
> > {
> >   /* assume input: "1<>\0" */
> >   while (*f != '\0')
> >     {
> >       const unsigned char ** pstr = (const unsigned char **) &f;
> 
> comes from
> https://sourceware.org/git/?p=glibc.git;a=blob;f=stdio-common/vfscanf-internal.c;h=95b46dcbeb55b1724b396f02a940f3047259b926;hb=HEAD#l489
> :
> 
> """
>  274 int
>  275 __vfscanf_internal (FILE *s, const char *format, va_list argptr,
>  276                     unsigned int mode_flags)
> ...
>  487       if (ISDIGIT ((UCHAR_T) *f))
>  488         {
>  489           argpos = read_int ((const UCHAR_T **) &f);
> """

From this little context it eventually makes sense to declare
'f' as const unsigned char * in this function.
Comment 5 Andreas Schwab 2020-10-01 10:57:28 UTC
Why doesn't gcc warn about that?
Comment 6 Richard Biener 2020-10-01 11:31:31 UTC
(In reply to Andreas Schwab from comment #5)
> Why doesn't gcc warn about that?

It does:

unsigned char **q;
void foo (char *p)
{
  q = (unsigned char **)&p;
}

> gcc t.c -fstrict-aliasing -Wstrict-aliasing=2 -S
t.i: In function 'foo':
t.i:4:3: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
   q = (unsigned char **)&p;
   ^

note the default level of -Wstrict-aliasing when enabled is 3 which will
not warn when the pointer is not dereferenced in the same expression.
Comment 7 Richard Biener 2020-10-16 12:09:40 UTC
Invalid.