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

[Michael Gottesman]

After some discussion with Richard, it was agreed that the right thing to do here is to disable the redeclaring functionality only for builtins that use custom type checking since to quote the great zygoloid: “its hard to see how that could be a reasonable thing for user code to do”.

Preparing another patch.
Michael

On Oct 31, 2013, at 5:08 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> 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/20131101/d61e01a4/attachment.html>