[IR] Add missing GlobalAlias copying of ThreadLocalMode attribute

[nextsilicon-itay-bookstein]

Summary

Previously, GlobalAlias::copyAttributesFrom did not preserve ThreadLocalMode,
causing incorrect IR generation in IR linking flows. This patch pushes the code
responsible for copying this attribute from GlobalVariable::copyAttributesFrom
down to GlobalValue::copyAttributesFrom so that it is shared by GlobalAlias.
Fixes PR46297.

[tejohnson]

It looks like this is on an old version of clang, as this method is now in GlobalIndirectSymbol. You'll want to check this behavior and submit a patch for the latest clang. For the test, you could probably add an llvm-link based test in llvm/test/Linker/.

llvm/include/llvm/IR/GlobalAlias.h

63

It seems like this belongs in GlobalValue::copyAttributesFrom, since it is a property of the GlobalValue.

Interestingly I do see code in the IRMover (used when linking IR in LTO) that copies it over (in llvm/lib/Linker/IRMover.cpp). Are we missing places there? In any case, it seems reasonable to add this into GlobalValue::copyAttributesFrom.

[ibookstein]

Added inline reply about IRMover and where the copying belongs.

I think it least bug-prone to push the ThreadLocalMode copying from GlobalVariable::copyAttributesFrom down to GlobalValue::copyAttributesFrom.
Values in the hierarchy for which the attribute is not relevant will ignore it. What do you think?

Regarding patch applicability to master:
You're right, I prepared the patch on an LLVM-9-derived branch. Will fix according to the decided-upon changes, and add the relevant test.

llvm/include/llvm/IR/GlobalAlias.h

63

Regarding where it belongs:
At first I put it in GlobalValue::copyAttributesFrom, but then I saw that that exact line already was in GlobalVariable::copyAttributesFrom. So I'd assumed that it's intentionally put only in derived classes for which that attribute is relevant (i.e. GlobalVariable and GlobalAlias, but not GlobalIFunc and Function). I see no harm in putting it in the base class where it's defined, but I defer to your judgement.

Regarding the code in IRMover:
I do indeed see code that passes the source value's ThreadLocalMode, but only to GlobalVariable constructors. It seems there's a slight non-uniformity of interface between the constructors in that respect, as the GlobalAlias constructors (or rather, static factory methods) don't expose a way to pass the ThreadLocalMode at construction. The code in IRMover that creates GlobalAliases does not touch the ThreadLocalMode, and then static-dispatches to GlobalIndirectSymbol::copyAttributesFrom, which in turn just forwards the call to GlobalValue::copyAttributesFrom (where there's no handling for the ThreadLocalMode).

[tejohnson]

llvm/include/llvm/IR/GlobalAlias.h

63

I agree that since this property is defined on the GlobalValue it makes most sense to copy it in the GlobalValue::copyAttributesFrom. I am not an expert on this particular attribute though. I discovered that Hans Wennborg added it some years ago (circa 2012-2014!). Added him to the review here to comment. I'm guessing that it was meant to apply to variables but leaving it off of aliases was an oversight?

[nextsilicon-itay-bookstein]

Changed the fix to push the ThreadLocalMode copying from derived class GlobalVariable down to base class GlobalValue.
Added an appropriate test under llvm/test/Linker.

[tejohnson]

LGTM. If @hans has any comments they can be addressed in a follow on change. Two things to fix before submitting though:

• A small change to the test comments is needed as noted below before submitting.
• The patch description needs updating. Doesn't need a full test case etc like it currently has, but does just need a general overview of the problem and fix.

llvm/test/Linker/alias-threadlocal.ll

2 ↗

(On Diff #270093)

Please fix the bug number. https://bugs.llvm.org/show_bug.cgi?id=81605 is not a valid bug. In addition, please add a one sentence description at the top of what this is testing.

[nextsilicon-itay-bookstein]

Updated title and description.
Added description for test, created PR in bugzilla, corrected PR reference to point at it.

While creating this PR, I noted that this looks slightly related to https://bugs.llvm.org/show_bug.cgi?id=33846, but I am yet to check whether they are truly related or not. It doesn't mention aliases or anything of the sort, and changing linkers worked around the issue there.

[hans]

In D81605#2088652, @tejohnson wrote:

LGTM. If @hans has any comments they can be addressed in a follow on change.

When I added the TLS mode attribute, it only applied to GlobalVariables. Later, in 59f7eba2b546e4118434b0ea9de5e08c316a64b1, it was moved to also apply to aliases, but it seems that commit forgot to move the attribute copying. The current patch looks like a good fix to me.

[nextsilicon-itay-bookstein]

Alright, looks like it's settled and ready then :)
I cannot commit the patch myself because I don't have commit access, so might I trouble one of you with that?

In another PR I was asked for an Author line for the git commit, including it preemptively.

Author: Itay Bookstein <[e-mail]>

llvm/include/llvm/IR/GlobalAlias.h

public:
   static GlobalAlias *create(LinkageTypes Linkage, const Twine &Name,
                              GlobalValue *Aliasee);
 
   // Linkage, Type, Parent and AddressSpace taken from the Aliasee.
   static GlobalAlias *create(const Twine &Name, GlobalValue *Aliasee);
 
   void copyAttributesFrom(const GlobalValue *Src) {
     GlobalValue::copyAttributesFrom(Src);
+    setThreadLocalMode(Src->getThreadLocalMode());
   }
 
   /// removeFromParent - This method unlinks 'this' from the containing module,
   /// but does not delete it.
   ///
   void removeFromParent();
 
   /// eraseFromParent - This method unlinks 'this' from the containing module