[PATCH] Only allow builtin/runtime builtins to be forgotten upon seeing a declaration with the same name.

[Richard Smith]

On Thu, Oct 31, 2013 at 4:27 PM, Michael Gottesman <mgottesman at apple.com>wrote:

> Hey dgregor!
>
> The attached patch ensures that clang only allows the user to respecify
> declarations for library/runtime builtins instead of generic builtins.
>

We explicitly allow people to redeclare builtins, and they apparently do
so. We don't want to just ban this outright.


> Background:
> In r122351, clang began to allow users to override the prototype of
> builtins if the user declared a function with the same name. The idea
> behind this was to give implementors of common library/runtime functions
> clang knows about the flexiblity to use different prototypes than what
> clang has been taught to expect.
>
> This patch restricts such behavior in order to prevent the users from
> overriding general compiler builtins since said builtins via the "t"
> specifier allow one to do (essentially) what ever one wants by specifying
> that the builtin has custom type checking/special implementation.
>

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:

  extern "C" int __builtin_abs(const char*);
  int n = __builtin_abs("foo");

the call to __builtin_abs will be treated as a call to a builtin. I expect
that'll crash.


> The example inspiring this change is the following:
>
> namespace test {
>   int __sync_add_and_fetch_4(int *p, int n) { return 0; }
>

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.

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.


>   int syncAddAndFetch(volatile int *p, int n) {
>     return __sync_add_and_fetch(p, n);
>   }
> }
>
> What occurs here is that clang sees __sync_add_and_fetch_4 and more
> importantly that the prototype differs from the prototype that clang has
> from Builtins.def:
>
>   int __sync_add_and_fetch_4(volatile int *p, int n, ...)
>
> Then clang erase its original builtin definition under the impression
> that we are overriding said builtin.
>
> Then when clang processes __sync_add_and_fetch, it generates calls to
> __sync_add_and_fetch_4 as if it was still a builtin. This then causes an
> unreachable to be hit in CodeGen when CodeGen attempts to process an
> ImplicitCast of the DeclRef of the function call as if it is not a
> builtin only to find that the ImplicitCast is of BuiltinFnToFnPtr.
>
> Please review,
> Michael
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131031/2d3b95de/attachment.html>