<div dir="ltr">On Thu, Oct 31, 2013 at 4:27 PM, Michael Gottesman <span dir="ltr"><<a href="mailto:mgottesman@apple.com" target="_blank">mgottesman@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hey dgregor!<br>
<br>
The attached patch ensures that clang only allows the user to respecify declarations for library/runtime builtins instead of generic builtins.<br></blockquote><div><br></div><div>We explicitly allow people to redeclare builtins, and they apparently do so. We don't want to just ban this outright.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Background:<br>
In r122351, clang began to allow users to override the prototype of<br>
builtins if the user declared a function with the same name. The idea<br>
behind this was to give implementors of common library/runtime functions<br>
clang knows about the flexiblity to use different prototypes than what<br>
clang has been taught to expect.<br>
<br>
This patch restricts such behavior in order to prevent the users from<br>
overriding general compiler builtins since said builtins via the "t"<br>
specifier allow one to do (essentially) what ever one wants by specifying<br>
that the builtin has custom type checking/special implementation.<br></blockquote><div><br></div><div>The patch doesn't look right to me. If the user declares (but does not define) the builtin, and uses a bogus type, you're causing the user's function to be treated as a builtin. For instance, if the user writes:</div>
<div><br></div><div>  extern "C" int __builtin_abs(const char*);</div><div>  int n = __builtin_abs("foo");</div><div><br></div><div>the call to __builtin_abs will be treated as a call to a builtin. I expect that'll crash.</div>
<div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The example inspiring this change is the following:<br>
<br>
namespace test {<br>
  int __sync_add_and_fetch_4(int *p, int n) { return 0; }<br></blockquote><div><br></div><div>This should not have caused us to forget the builtin, because it is declared in a namespace (and isn't extern "C"). getBuiltinID should have returned 0 here, so we should not have reached the 'forgetting' code.</div>
<div><br></div><div>If you remove the namespace, you do have a problem here: we incorrectly forget the builtin (this is wrong because this is not an overload of it). As it happens, Warren Hunt has been working on a patch which should fix that case.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  int syncAddAndFetch(volatile int *p, int n) {<br>
    return __sync_add_and_fetch(p, n);<br>
  }<br>
}<br>
<br>
What occurs here is that clang sees __sync_add_and_fetch_4 and more<br>
importantly that the prototype differs from the prototype that clang has<br>
from Builtins.def:<br>
<br>
  int __sync_add_and_fetch_4(volatile int *p, int n, ...)<br>
<br>
Then clang erase its original builtin definition under the impression<br>
that we are overriding said builtin.<br>
<br>
Then when clang processes __sync_add_and_fetch, it generates calls to<br>
__sync_add_and_fetch_4 as if it was still a builtin. This then causes an<br>
unreachable to be hit in CodeGen when CodeGen attempts to process an<br>
ImplicitCast of the DeclRef of the function call as if it is not a<br>
builtin only to find that the ImplicitCast is of BuiltinFnToFnPtr.<br>
<br>
Please review,<br>
Michael<br>
<br>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div>