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

[jahanian]

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:

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/20140523/8fb9eb95/attachment.html>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch.txt
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140523/8fb9eb95/attachment.txt>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140523/8fb9eb95/attachment-0001.html>