[cfe-dev] clang::CodeGen::CodeGenModule::GetOrCreateLLVMGlobal

[Vassil Vassilev]

Hi Rafael,
   Do you think we could rely for this case on post commit review?
Cheers,
Vassil
On 04/24/2014 08:52 PM, Rafael Espíndola wrote:
> ping 3
>
> On 24 March 2014 00:01, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
>> ping 2
>>
>> On 17 March 2014 06:59, Vassil Vassilev <vasil.georgiev.vasilev at cern.ch> wrote:
>>> ping...
>>>
>>> On 03/11/2014 01:05 PM, Rafael Espíndola wrote:
>>>>> I am not sure I understand the intended semantics of the UnnamedAddr
>>>>> argument to GetOrCreateLLVMGlobal.  Is it “this use does not care about the
>>>>> specific address”?  Because, if so, the (existing) logic is completely
>>>>> backwards: we need to be *clearing* the flag on the global if the argument
>>>>> is ever *false*.  And that seems like the most reasonable semantics,
>>>>> frankly.
>>>>>
>>>>> Also, the idea in the existing code that all runtime variables are
>>>>> unnamed_addr seems bogus to me.  __dso_handle is a specific example of a
>>>>> variable whose address value *is* semantically important.  This sort of
>>>>> thing would be important if there were any plausible transformations at all
>>>>> that involve unnamed_addr on external variable declarations.
>>>>>
>>>>> Also, the assertion is wrong: if something introduces a “runtime"
>>>>> declaration (e.g. by just declaring and using a global variable with the
>>>>> right name) before CodeGen emits its first intrinsic use of it, the
>>>>> declaration will not be marked unnamed_addr.
>>>>>
>>>>> So, basically, right now, this entire code path is useless and wrong.  Is
>>>>> there a plan to apply it in situations where it wouldn’t be useless, i.e. to
>>>>> non-runtime user declarations?
>>>> So, on how the patch is organized:
>>>>
>>>> * The structure the patch uses is that when the variable is first
>>>> created, its unnamed_addr is set to the value of the argument.
>>>> * We then check that no user expect that a variable is unnamed_addr
>>>> when in fact it isn't. I tried to make the check more strict by
>>>> asserting that Entry->getUnnamedAddr() == UnnamedAddr, but that fails
>>>> deep down in some objc cases that create runtime variables and access
>>>> them as regular ones.
>>>>
>>>> But to the more important point: Should we be adding unnamed_addr in
>>>> the first place? I am not sure. Back in
>>>>
>>>> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110110/037796.html
>>>> Chris asked if we should. I could not find the entire discussion (the
>>>> patch that ended up being committed is simpler) and I am still not
>>>> sure exactly when CreateRuntimeVariable should be used.
>>>>
>>>> An alternative patch that simply never sets it in
>>>> CreateRuntimeVariable is attached.
>>>>
>>>> Cheers,
>>>> Rafael
>>>