[PATCH] D122608: Fix behavior of ifuncs with 'used' extern "C" static functions

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6323-6325
+  // If there is more than 1 use, I don't really know what we can do.
+  // EmitStaticExternCAliases won't be able to do anything, and the ifunc
+  // diagnostic will have to catch this.
----------------
tahonermann wrote:
> This comment doesn't state what the challenge is when there is more than one use. Is the concern that there may be multiple ifuncs that have the same associated resolver? What might cause that to happen? (presumably multiple functions declared with the `ifunc` attribute that name the same resolver).
> 
> A test that exercises this case seems appropriate.
I think the 'multiple ifuncs' isn't actually an issue, it is more that "I have no idea what else could use this/create this".  Thinking now, the multiple 'ifuncs' seems completely doable now.  I'll update the patch to handle multiples here.


================
Comment at: clang/test/SemaCXX/externc-ifunc-resolver.cpp:8
+  }
+  __attribute__((ifunc("resolve_foo"))) void foo(); // expected-error{{ifunc must point to a defined function}}
+}
----------------
tahonermann wrote:
> There is considerable subtlety between these two tests and, unfortunately, the diagnostic isn't particularly helpful.
> 
> If I'm following correctly, the other test passes because an unmangled name is specified for the `ifunc` attribute and that is resolved against the emitted alias.
> 
> In this test, the same code is present as the other test, but with an additional declaration that doesn't seem relevant; one would not expect lookup for "resolve_foo" in the `ifunc` attribute to include the `NS` namespace. Is the error generated because two aliases for "resolve_foo" are emitted? If so, that seems like something that should be diagnosed somewhere. Or perhaps an alias should not be emitted for `used` functions defined at namespace scope.
>>If I'm following correctly, the other test passes because an unmangled name is specified for the ifunc attribute and that is resolved against the emitted alias.

Yes, this is correct. 

>>There is considerable subtlety between these two tests and, unfortunately, the diagnostic isn't particularly helpful.

Yeah, this is unfortunate.  However, when it is diagnosed causes some problems in that we don't have the real name of the declaration available anymore.  I'm not intending to improve the quality of the diagnostic here though.

>> Is the error generated because two aliases for "resolve_foo" are emitted?
It is because two aliases WOULD be emitted, so we choose not to.

>> If so, that seems like something that should be diagnosed somewhere. 
Maybe?  Probably? I don't see the 'used' alias functionality as a part of this patch, I'm trying to keep that behavior as much as it possibly exists now.

>> Or perhaps an alias should not be emitted for used functions defined at namespace scope.
This we cannot do without breaking code that depends on it already.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122608/new/

https://reviews.llvm.org/D122608