Patch to fix an invalid AST of a locally redeclared built-in which causes an IRGen crash

[Richard Smith]

On Fri, May 23, 2014 at 4:36 PM, jahanian <fjahanian at apple.com> wrote:

>
> On May 22, 2014, at 1:58 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>
> On Thu, May 22, 2014 at 1:48 PM, jahanian <fjahanian at apple.com> wrote:
>
>>
>> On May 22, 2014, at 11:40 AM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>>
>>
>> Another problem with our current model is that we build a broken
>> redeclaration chain (the local declaration is marked as being a
>> redeclaration of the implicitly-declared builtin, which it isn't). This
>> leads to other bugs; for instance:
>>
>> void f() { int memcpy(void); } void g() { memcpy(0,0,0); }
>>
>> ... misses the "implicitly declaring library function" warning.
>>
>>
>> Interesting point. Currently, we create a built-in declaration of
>> “memcpy” and make local declaration as its redeclaration.
>> Should we just not create the implicit built-in declaration when user
>> declaration is local? If we do this,
>> then the missing  warning you mentioned will come out (and my bug gets
>> fixed). But, we will miss the warning
>> about "incompatible redeclaration of library function ‘memcpy’” on
>> local “redeclaration”.
>> Which, I think, is ok as it is no longer a redeclaration.
>> Am I reading you correctly?
>>
>
> I think we should still produce the "incompatible redeclaration of library
> function" warning. I think it'd be better if we didn't create the implicit
> built-in declaration at all, but I don't think it should matter whether the
> declaration is local. If I write a non-local declaration:
>
>
> I have an experimental patch which does not create the  implicit built-in
> declaration at all. It fixes my original bug and also produces the error
> for the test case that you sent earlier:
>

This direction looks very interesting, and might be a good solution.

I see this in your testcases:

-void exit();
+void exit(int) __attribute__((noreturn));

Presumably the problem is that we're no longer inheriting 'noreturn' from
the previous declaration (because there isn't one), and the test relies on
'exit' being noreturn. Is this limited to 'noreturn' (which affects the
function type), or does it affect all the other attributes too? In the
latter case, this is a serious problem -- it's not OK to drop the
'returns_twice' attribute of setjmp, because that causes us to miscompile
its callers.

Can we add the implied attributes (including noreturn and its change of
function type) if the declared function's type is compatible with the type
the builtin would have?

Also this:

-    char *rindex();
+    char *rindex(); // expected-warning {{incompatible redeclaration of
library function 'rindex'}} \
+    // expected-note {{'rindex' is a builtin with type 'char *(const char
*, int)'}}

This declaration *is* compatible with the builtin's type, isn't it?

 char *rindex(s, c)
-     register char *s, c; // expected-warning{{promoted type 'char *' of
K&R function parameter is not compatible with the parameter type 'const
char *' declared in a previous prototype}}
+     register char *s, c;

This ought to give the 'incompatible redeclaration' warning, I think?


+  // This diagnostic is now being issued in one central location.
+  if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID()))
       return false;

I think we should still issue a diagnostic here. If someone uses a builtin
(and causes its implicit declaration), then declares it with a different
signature, that should be hard error.


+          if (!T.isNull() && !Context.hasSameType(NewFD->getType(), T)) {

This "same type" check is the cause of some of the badness I pointed out
above. You should check that the types are compatible (and, if possible,
ignore the 'noreturn' attribute in the process).

fix.c:1:16: warning: incompatible redeclaration of library function 'memcpy'
> void f() { int memcpy(void); }
>                ^
> fix.c:1:16: note: 'memcpy' is a builtin with type 'void *(void *, const
> void *, unsigned long)'
> fix.c:3:12: warning: use of out-of-scope declaration of 'memcpy'
> void g() { memcpy(0,0,0); }
>            ^
> fix.c:1:16: note: previous declaration is here
> void f() { int memcpy(void); }
>                ^
> fix.c:3:19: error: too many arguments to function call, expected 0, have 3
> void g() { memcpy(0,0,0); }
>            ~~~~~~ ^~~~~
> fix.c:1:12: note: 'memcpy' declared here
> void f() { int memcpy(void); }
>            ^
> 2 warnings and 1 error generated.
>
> But there are several issues remaining.
>
> 1) This patch issues a diagnostic only once. So, for example, for this
> test:
> void f() { int memcpy(void); }
> void g() { int memcpy(void); }
>
> or:
> void f() { int memcpy(void); }
> int memcpy(void) {}
>
> We now issue the warning on first declaration of memcpy. Reason being that
> after 1st declaration we
> treat memcpy as a user declaration and the 2nd declaration will not be
> seen as built-in
> (you mentioned that there is a Builtin context I can check without
> requiring BuiltinID. But I
> cannot find it).
>
> Many tests had to be modified mainly because global declaration of builtin
> functions in these tests are
> incorrect and this patch catches it. But this also points that projects
> will break.
>
> Patch is attached for your further comments.
> - Thanks, Fariborz
>
>
>
>
>
>
>
>   int memcpy(void);
>
> ... I should not get a redecl chain for memcpy with two declarations with
> different parameter types.
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140529/52b0bb3b/attachment.html>