[PATCH] CodeGen: __uuidof should work even with an incomplete _GUID type

[Reid Kleckner]

On Tue, Aug 13, 2013 at 5:35 PM, Richard Smith <richard at metafoo.co.uk>wrote:

> On Tue, Aug 13, 2013 at 1:59 PM, David Majnemer <david.majnemer at gmail.com>wrote:
>
>>
>>
>> ================
>> Comment at: test/CodeGenCXX/microsoft-uuidof.cpp:8
>> @@ +7,3 @@
>> +#ifdef WRONG_GUID
>> +    unsigned int SomethingWentWrong[4];
>> +#else
>> ----------------
>> Reid Kleckner wrote:
>> > I was expecting a diagnostic.  :)
>> >
>> > Previously clang would reject this code:
>> > struct __declspec(uuid("12345678-1234-1234-1234-1234567890aB")) S { };
>> > typedef struct _GUID { int x; int y; } GUID;
>> > GUID g = __uuidof(S);
>> >
>> > cl.exe accepts and only copies the first 8 bytes, but I'd like to
>> preserve the original clang behavior.  Can we at least check that the
>> sizeof() the user's type is 16?
>> Are we going into the business of making sure system-types are well
>> formed? It isn't really a user type, it starts with an underscore and a
>> capital letter... We don't make sure the std::type_info type is proper.
>>
>> That said, consider it done if you guys feel strongly about it.
>
>
> I'll let Reid make the call here: I don't think it matters much, since
> _GUID is a system type, and if someone defines it themselves and gets it
> wrong, they'll get broken (but technically correct) code rather than an ICE.
>

OK, how about a test to ensure that the memcpy doesn't overrun the type?
 That's what cl does at least.


> ================
>> Comment at: lib/CodeGen/CodeGenModule.cpp:1063
>> @@ -1074,3 +1062,3 @@
>>        /*isConstant=*/true, llvm::GlobalValue::ExternalLinkage, Init,
>> Name);
>>    GV->setUnnamedAddr(true);
>>    return GV;
>> ----------------
>> Richard Smith wrote:
>> > Is unnamed_addr really correct for these things?
>> That was there when I got here. ;)
>>
>> LangRef makes it sound like appropriate for this case:
>> > Global variables can be marked with unnamed_addr which indicates that
>> the address is not significant, only the content. Constants marked like
>> this can be merged with other constants if they have the same initializer.
>> Note that a constant with significant address can be merged with a
>> unnamed_addr constant, the result being a constant whose address is
>> significant."
>>
>> What was your thinking?
>
>
> We expose the address of the __s_GUID object to user code, so it doesn't
> seem like it should be unnamed_addr. For instance:
>
> const char my_stuff[16] = { the GUID };
> assert(my_stuff != &__uuidof(type));
>
> That assert should presumably not fire.
>

I agree.  At least it's more conservative.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130813/2c77cbc3/attachment.html>