[PATCH] D22642: CodeGen: Clean up implementation of vtable initializer builder. NFC.

[Richard Smith via cfe-commits]

On Wed, Sep 7, 2016 at 7:14 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> On 7 Sep 2016 6:59 pm, "Peter Collingbourne" <peter at pcc.me.uk> wrote:
>
> On Wed, Sep 7, 2016 at 6:44 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>
>> On 7 Sep 2016 6:23 pm, "Peter Collingbourne" <peter at pcc.me.uk> wrote:
>>
>> pcc marked 4 inline comments as done.
>>
>> ================
>> Comment at: lib/CodeGen/CGVTables.cpp:588
>> @@ +587,3 @@
>> +        if (auto *F = dyn_cast<llvm::Function>(Cache))
>> +          F->setUnnamedAddr(llvm::GlobalValue::UnnamedAddr::Global);
>> +        Cache = llvm::ConstantExpr::getBitCast(Cache, CGM.Int8PtrTy);
>> ----------------
>> rsmith wrote:
>> > Do you have any idea why we're doing this? It looks wrong to me. These
>> ABI entry points are exposed and could certainly have their addresses taken
>> and used in this translation unit.
>> I introduced this in D18071. Although a translation unit can take the
>> address of one of these functions, that would involve declaring a function
>> with a reserved name, so I believe we'd be allowed to impose restrictions
>> such as `unnamed_addr` on the address.
>>
>>
>> These are declared by <cxxabi.h>, and thus presumably intended to be
>> useable by programs, so I'm not convinced that reasoning applies.
>>
>
> cxxabi.h isn't part of the standard, is it? So whatever it does shouldn't
> have an effect on the standard.
>
>
> It's part of the itanium c++ abi standard.
>

OK, so this is actually not completely clear. On the one hand,
__cxa_pure_virtual is declared by GCC's <cxxabi.h> and libc++abi's
<cxxabi.h>, and documented in the ABI description as a function that is
part of the API provided by the implementation, but on the other hand, it's
not part of the "reference implementation" of <cxxabi.h> provided as part
of the Itanium C++ ABI.

Reasonable code is not going to use the address of this function, and even
in unreasonable code, the only really plausible use would be comparing the
address of __cxa_pure_virtual to the contents of a vtable slot, which is
already not guaranteed to work since implementations aren't required to put
that function pointer there.

Do we gain anything from this?
>>
>
> Not at present (the relative ABI is the only user I know of and that isn't
> fully implemented yet on trunk), although something like this may be
> required if we ever fully implement the relative ABI.
>
>
Since the idea here is to allow use of a relative address for the function,
it strikes me that there's another option: we could emit an unnamed_addr
linkonce_odr hidden visibility forwarding thunk for the function
(effectively, doing the lowering to a PLT thunk in the frontend). That'd
let us form a relative address for the function without disturbing other
uses of the symbol. However, the complexity of that solution doesn't seem
justified by the (largely theoretical) benefit to me.

Let's leave this alone, then. (We could presumably restrict it to just the
relative-ABI case, but I think we'd prefer to find out sooner if this
actually causes problems for someone.)

It also seems like there's an LLVM IR deficiency here to me -- there's
apparently no way to state that one particular use of a global is
unnamed_addr without making the global itself be unnamed_addr.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160908/42f8a6eb/attachment.html>