Re: segfault in vfscanf(3): clang and __restrict usage

On 2012-04-24 21:49, Jean-Sébastien Pédron wrote:
> Hi everyone,
> 
> vfscanf(3) in HEAD (r234606) segfaults when compiled with clang. For
> instance, here is a call made in cmake which crashes:
>     fscanf(f, "%*[^\n]\n");

Using r234549 here, everything compiled with clang, but I cannot make
that statement crash, whatever I do.  Do you have a specific input file
which crashes it?


> The same libc, compiled with GCC, doesn't segfault.
> 
> When it encounters a character class, __svfscanf() calls convert_ccl():
> 
> static const int suppress;
> #define SUPPRESS_PTR    ((void *)&suppress)
> 
> static __inline int
> convert_ccl(FILE *fp, char * __restrict p, [...])
> {
>         [...]
> 
>         if (p == SUPPRESS_PTR) {
> 		[...]
> 	} else {
> 		[...]
> 	}
> 
> 	[...]
> }
...
> From what I understand about __restrict, it indicates that the pointer
> is the only one pointing to a resource. In vfscanf.c, "suppress" may
> be pointed by several pointers at a time, so I think __restrict here
> is incorrect. But I'm really not sure I got it right. And I don't know
> either if clang behavior is expected.

Indeed, my first impression was the same, that the use of 'restrict' is
wrong here.  Namely, if you tell the compiler that 'p' is the *only*
pointer pointing to a specific object, and you compare it with any other
pointer, the comparison will be unequal by definition.

However, after asking around a bit, it seems that clang is still wrong
in this particular case.  Although this is probably language lawyer
area, so beware. :)

I have filed an LLVM PR for it here:
  
  http://llvm.org/bugs/show_bug.cgi?id=12656 

Meanwhile, I really wonder why the __restrict keyword was used in this
implementation.  There are lots of cases in vfscanf.c, where a pointer
is declared __restrict, and then aliasing seems to be done anyway. 

Besides, I'm not really sure about the potential optimization gains of
adding the keyword.  With our base gcc, removing all the __restrict
keywords results in no binary change.  With gcc 4.7, there are some very
minor changes, but they are extremely unlikely to gain any performance.

And with clang, there are quite some differences, but apparently it
optimizes too aggressively, so more testing is required to see if the
potential for bugs outweighs the performance gains.