<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, May 23, 2014 at 4:36 PM, jahanian <span dir="ltr"><<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div class="h5">
<div>On May 22, 2014, at 1:58 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
On Thu, May 22, 2014 at 1:48 PM, jahanian <span dir="ltr"><<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div>On May 22, 2014, at 11:40 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>> wrote:</div>

<br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>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:</div>


<div><br></div><div>void f() { int memcpy(void); } void g() { memcpy(0,0,0); }<br></div><div><br></div><div>... misses the "implicitly declaring library function" warning.</div></div></div></div></blockquote><div>

<br></div></div>Interesting point. Currently, we create a built-in declaration of “memcpy” and make local declaration as its redeclaration.</div><div>Should we just not create the implicit built-in declaration when user declaration is local? If we do this,</div>

<div>then the missing  warning you mentioned will come out (and my bug gets fixed). But, we will miss the warning </div><div>about "<font face="Menlo"><span style="font-size:11px">incompatible redeclaration of library function ‘memcpy’” on local “redeclaration”.</span></font></div>

<div><font face="Menlo"><span style="font-size:11px">Which, I think, is ok as it is no longer a redeclaration.</span></font></div><div><font face="Menlo"><span style="font-size:11px">Am I reading you correctly?</span></font></div>

</div></blockquote><div><br></div><div>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:</div>
</div></div></div></blockquote><div><br></div></div></div>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</div><div>for the test case that you sent earlier:</div>
</div></blockquote><div><br></div><div>This direction looks very interesting, and might be a good solution.</div><div><br></div><div>I see this in your testcases:</div><div><br></div><div><div>-void exit();</div><div>+void exit(int) __attribute__((noreturn));</div>
</div><div><br></div><div>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.</div>
<div><br></div><div>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?</div><div><br></div><div>Also this:</div>
<div><br></div><div><div>-    char *rindex();</div><div>+    char *rindex(); // expected-warning {{incompatible redeclaration of library function 'rindex'}} \</div><div>+<span class="" style="white-space:pre">           </span>    // expected-note {{'rindex' is a builtin with type 'char *(const char *, int)'}}</div>
</div><div><br></div><div>This declaration *is* compatible with the builtin's type, isn't it?</div><div><br></div><div><div> char *rindex(s, c)</div><div>-     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}}</div>
<div>+     register char *s, c;</div></div><div><br></div><div>This ought to give the 'incompatible redeclaration' warning, I think?</div><div><br></div><div><br></div><div><div>+  // This diagnostic is now being issued in one central location.</div>
<div>+  if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID()))</div><div>       return false;</div></div><div><br></div><div>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.</div>
<div><br></div><div><br></div><div>+          if (!T.isNull() && !Context.hasSameType(NewFD->getType(), T)) {<br></div><div><br></div><div>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).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div style="margin:0px;font-size:11px;font-family:Menlo">
fix.c:1:16: warning: incompatible redeclaration of library function 'memcpy'</div><div style="margin:0px;font-size:11px;font-family:Menlo">void f() { int memcpy(void); } </div><div style="margin:0px;font-size:11px;font-family:Menlo">
               ^</div><div style="margin:0px;font-size:11px;font-family:Menlo">fix.c:1:16: note: 'memcpy' is a builtin with type 'void *(void *, const void *, unsigned long)'</div><div style="margin:0px;font-size:11px;font-family:Menlo">
fix.c:3:12: warning: use of out-of-scope declaration of 'memcpy'</div><div style="margin:0px;font-size:11px;font-family:Menlo">void g() { memcpy(0,0,0); }</div><div style="margin:0px;font-size:11px;font-family:Menlo">
           ^</div><div style="margin:0px;font-size:11px;font-family:Menlo">fix.c:1:16: note: previous declaration is here</div><div style="margin:0px;font-size:11px;font-family:Menlo">void f() { int memcpy(void); } </div>
<div style="margin:0px;font-size:11px;font-family:Menlo">               ^</div><div style="margin:0px;font-size:11px;font-family:Menlo">fix.c:3:19: error: too many arguments to function call, expected 0, have 3</div><div style="margin:0px;font-size:11px;font-family:Menlo">
void g() { memcpy(0,0,0); }</div><div style="margin:0px;font-size:11px;font-family:Menlo">           ~~~~~~ ^~~~~</div><div style="margin:0px;font-size:11px;font-family:Menlo">fix.c:1:12: note: 'memcpy' declared here</div>
<div style="margin:0px;font-size:11px;font-family:Menlo">void f() { int memcpy(void); } </div><div style="margin:0px;font-size:11px;font-family:Menlo">           ^</div><div style="margin:0px;font-size:11px;font-family:Menlo">
2 warnings and 1 error generated.</div><div style="margin:0px;font-size:11px;font-family:Menlo"><br></div><div style="margin:0px;font-size:11px;font-family:Menlo">But there are several issues remaining.</div><div style="margin:0px;font-size:11px;font-family:Menlo">
<br></div><div style="margin:0px;font-size:11px;font-family:Menlo">1) This patch issues a diagnostic only once. So, for example, for this test:</div><div style="margin:0px;font-size:11px;font-family:Menlo"><div style="margin:0px">
void f() { int memcpy(void); } </div><div style="margin:0px">void g() { int memcpy(void); }</div><div style="margin:0px"><br></div><div style="margin:0px">or:</div><div style="margin:0px"><div style="margin:0px">void f() { int memcpy(void); } </div>
<div style="margin:0px">int memcpy(void) {}</div></div><div style="margin:0px"><br></div><div style="margin:0px">We now issue the warning on first declaration of memcpy. Reason being that after 1st declaration we</div><div style="margin:0px">
treat memcpy as a user declaration and the 2nd declaration will not be seen as built-in</div><div style="margin:0px">(you mentioned that there is a Builtin context I can check without requiring BuiltinID. But I</div><div style="margin:0px">
cannot find it).</div><div style="margin:0px"><br></div><div style="margin:0px">Many tests had to be modified mainly because global declaration of builtin functions in these tests are</div><div style="margin:0px">incorrect and this patch catches it. But this also points that projects will break.</div>
<div style="margin:0px"><br></div><div style="margin:0px">Patch is attached for your further comments.</div><div style="margin:0px">- Thanks, Fariborz</div><div style="margin:0px"></div></div></div></div><br><div style="word-wrap:break-word">
<div><div style="margin:0px;font-size:11px;font-family:Menlo"><div style="margin:0px"></div><div style="margin:0px"><br></div><div style="margin:0px"><br></div><div style="margin:0px"><br></div><div style="margin:0px"><br>
</div></div><div style="margin:0px;font-size:11px;font-family:Menlo"><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<div><br></div><div>  int memcpy(void);</div><div><br></div><div>... I should not get a redecl chain for memcpy with two declarations with different parameter types.</div></div></div></div>
</blockquote></div><br></div>
<br></blockquote></div><br></div></div>