[llvm-commits] [llvm] r107205 - in /llvm/trunk: bindings/ada/llvm/ docs/ include/llvm-c/ include/llvm/ include/llvm/CodeGen/ include/llvm/MC/ include/llvm/Target/ lib/AsmParser/ lib/Bitcode/Reader/ lib/Bitcode/Writer/ lib/CodeGen/ lib/CodeGen/AsmPrinter/ lib/Linker/ lib/MC/ lib/Target/ lib/Target/CppBackend/ lib/Target/XCore/AsmPrinter/ lib/Transforms/IPO/ lib/VMCore/ tools/llvm-nm/

[Bill Wendling]

On Jun 29, 2010, at 2:56 PM, Chris Lattner wrote:

> On Jun 29, 2010, at 2:24 PM, Bill Wendling wrote:
> 
>> Author: void
>> Date: Tue Jun 29 16:24:00 2010
>> New Revision: 107205
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=107205&view=rev
>> Log:
>> Introducing the "linker_weak" linkage type. This will be used for Objective-C
>> metadata types which should be marked as "weak", but which the linker will
>> remove upon final linkage.
> 
> Hi Bill,
> 
> This would have been good to talk about before you implemented it.  The name linker_weak doesn't seem very descriptive.  How about linker_private_weak or something like that?
> 
I thought about that, but they aren't private from what I can tell. They're marked as ".globl" and ".weak_definition".

>> +++ llvm/trunk/docs/LangRef.html Tue Jun 29 16:24:00 2010
>> 
>>  <dt><tt><b><a name="linkage_linker_private">linker_private</a></b></tt></dt>
>> +  <dd>Similar to <tt>private</tt>, but the symbol is passed through the
>> +      assembler and removed by the linker after evaluation.  Note that (unlike
>> +      <tt>private</tt> symbols) <tt>linker_private</tt> symbols are subject to
>> +      coalescing by the linker: weak symbols get merged and redefinitions are
>> +      rejected.  However, unlike normal strong symbols, they are removed by the
>> +      linker from the final linked image (executable or dynamic library).</dd>
> 
> This description doesn't make sense: linker_private symbols aren't weak.  The description of weak stuff should be moved to linker_private_weak.  
> 
I was quoting you from when I implemented linker_private. :) I'll try to make it more sensible.

>> +  <dt><tt><b><a name="linkage_linker_weak">linker_weak</a></b></tt></dt>
>> +  <dd>Global values with "<tt>linker_weak</tt>" linkage are given weak linkage,
>> +      but are removed by the linker after evaluation.</dd>
> 
> This should describe the weak semantics.
> 
>> +++ llvm/trunk/include/llvm/CodeGen/AsmPrinter.h Tue Jun 29 16:24:00 2010
>> @@ -285,7 +285,7 @@
>>    MCSymbol *GetCPISymbol(unsigned CPID) const;
>> 
>>    /// GetJTISymbol - Return the symbol for the specified jump table entry.
>> -    MCSymbol *GetJTISymbol(unsigned JTID, bool isLinkerPrivate = false) const;
>> +    MCSymbol *GetJTISymbol(unsigned JTID, bool PassToLinker = false) const;
> 
> "PassToLinker" doesn't mean anything, why did you rename it?

I renamed it because the name didn't mean what it used to after the addition of "linker_weak".

> Please rename it to be more descriptive or improve the comment.  "isLinkerPrivate" seems like a perfectly good and descriptive name to me.

It would be if we only had "linker_private" to worry about. If we don't expect linker_private_weak here, then it can be reverted.

>> project/llvm/trunk/include/llvm/MC/MCAsmInfo.h?rev=107205&r1=107204&r2=107205&view=diff
>> ==============================================================================
>> --- llvm/trunk/include/llvm/MC/MCAsmInfo.h (original)
>> +++ llvm/trunk/include/llvm/MC/MCAsmInfo.h Tue Jun 29 16:24:00 2010
>> @@ -85,6 +85,11 @@
>>    /// be passed through the assembler but be removed by the linker.  This
>>    /// is "l" on Darwin, currently used for some ObjC metadata.
>>    const char *LinkerPrivateGlobalPrefix;   // Defaults to ""
>> +
>> +    /// LinkerWeakGlobalPrefix - This prefix is used for symbols that are marked
>> +    /// "weak" and should be passed through the assembler, but be removed by the
>> +    /// linker.  This is "l" on Darwin, currently used for some ObjC metadata.
>> +    const char *LinkerWeakGlobalPrefix;      // Defaults to ""
> 
> Please make this "HasLinkerPrivateWeakLinkage".  There is only one target with this linkage form and it always spells it the same way.  Predicating stuff based on the bool is more clean considering that the *name* is the same as LinkerPrivate the linkage semantics require a new *directive* to be emitted.
> 
> In fact, I don't think you actually need this bit at all since asmprinter handles these symbols like all the other .weak stuff.  Is it needed?
> 
I'll check to see if it's used. If it's not used, then it's relying upon the hack down below, which is bad. However, I much prefer having the prefix string in one place than relying upon it being buried in some deep part of the asm printer.

>> 
>> +++ llvm/trunk/include/llvm/Target/Mangler.h Tue Jun 29 16:24:00 2010
>> @@ -32,7 +32,8 @@
>>  enum ManglerPrefixTy {
>>    Default,               ///< Emit default string before each symbol.
>>    Private,               ///< Emit "private" prefix before each symbol.
>> -    LinkerPrivate          ///< Emit "linker private" prefix before each symbol.
>> +    LinkerPrivate,         ///< Emit "linker private" prefix before each symbol.
>> +    LinkerWeak             ///< Emit "linker weak" prefix before each symbol.
> 
> This shouldn't be needed:  "LinkerWeak" and LinkerPrivate are mangled the same way, just use LinkerPrivate for LinkerPrivateWeak symbols.
>> 
>> +++ llvm/trunk/lib/AsmParser/LLLexer.cpp Tue Jun 29 16:24:00 2010
>> @@ -492,6 +492,7 @@
>> 
>>  KEYWORD(private);
>>  KEYWORD(linker_private);
>> +  KEYWORD(linker_weak);
> 
> Please also change the name in the .ll files.
> 
When you ask me to change the name at the beginning, you don't have to keep reminding me to change it everywhere. :-)

>> +++ llvm/trunk/lib/CodeGen/MachineFunction.cpp Tue Jun 29 16:24:00 2010
>> @@ -410,17 +410,18 @@
>> }
>> 
>> /// getJTISymbol - Return the MCSymbol for the specified non-empty jump table.
>> -/// If isLinkerPrivate is specified, an 'l' label is returned, otherwise a
>> -/// normal 'L' label is returned.
>> -MCSymbol *MachineFunction::getJTISymbol(unsigned JTI, MCContext &Ctx, 
>> -                                        bool isLinkerPrivate) const {
>> +/// If isLinkerPrivate or isLinkerWeak is specified, an 'l' label is returned,
>> +/// otherwise a normal 'L' label is returned.
>> +MCSymbol *MachineFunction::getJTISymbol(unsigned JTI, MCContext &Ctx,
>> +                                        bool PassToLinker) const {
>>  assert(JumpTableInfo && "No jump tables");
>> 
>>  assert(JTI < JumpTableInfo->getJumpTables().size() && "Invalid JTI!");
>>  const MCAsmInfo &MAI = *getTarget().getMCAsmInfo();
>> 
>> -  const char *Prefix = isLinkerPrivate ? MAI.getLinkerPrivateGlobalPrefix() :
>> -                                         MAI.getPrivateGlobalPrefix();
>> +  const char *Prefix = PassToLinker ?
>> +    MAI.getLinkerPrivateGlobalPrefix() :
>> +    MAI.getPrivateGlobalPrefix();
> 
> I don't see why you changed this stuff, it seems completely orthogonal to the rest of the patch.  Please change it back.
> 
How do you mean? Can't these be linker_weak (or linker_private_weak) as well?

>> +++ llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp Tue Jun 29 16:24:00 2010
>> @@ -755,11 +755,12 @@
>>  /// the directive emitted (this occurs in ObjC metadata).
>>  if (!GV) return false;
>> 
>> -  // Check whether the mangled name has the "Private" or "LinkerPrivate" prefix.
>> +  // Check whether the mangled name has the "Private", "LinkerPrivate", or
>> +  // "LinkerWeak" prefix.
>>  if (GV->hasLocalLinkage() && !isa<Function>(GV)) {
>>    // FIXME: ObjC metadata is currently emitted as internal symbols that have
>> -    // \1L and \0l prefixes on them.  Fix them to be Private/LinkerPrivate and
>> -    // this horrible hack can go away.
>> +    // \1L and \1l prefixes on them.  Fix them to be Private / LinkerPrivate /
>> +    // LinkerWeak and this horrible hack can go away.
>>    MCSymbol *Sym = Mang->getSymbol(GV);
>>    if (Sym->getName()[0] == 'L' || Sym->getName()[0] == 'l')
>>      return false;
> 
> I greatly look forward to this hack going away! :)
> 
Me too. :)

>> 
>> Modified: llvm/trunk/lib/Linker/LinkModules.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Linker/LinkModules.cpp?rev=107205&r1=107204&r2=107205&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Linker/LinkModules.cpp (original)
>> +++ llvm/trunk/lib/Linker/LinkModules.cpp Tue Jun 29 16:24:00 2010
>> @@ -735,6 +735,9 @@
>>  else if (SL == GlobalValue::LinkerPrivateLinkage &&
>>           DL == GlobalValue::LinkerPrivateLinkage)
>>    return GlobalValue::LinkerPrivateLinkage;
>> +  else if (SL == GlobalValue::LinkerWeakLinkage &&
>> +           DL == GlobalValue::LinkerWeakLinkage)
>> +    return GlobalValue::LinkerWeakLinkage;
>>  else {
>>    assert (SL == GlobalValue::PrivateLinkage &&
>>            DL == GlobalValue::PrivateLinkage && "Unexpected linkage type");
> 
> Is this really enough to cover all the .bc linking cases?
> 
>> +++ llvm/trunk/lib/Target/XCore/AsmPrinter/XCoreAsmPrinter.cpp Tue Jun 29 16:24:00 2010
>> @@ -129,6 +129,7 @@
>>  case GlobalValue::WeakAnyLinkage:
>>  case GlobalValue::WeakODRLinkage:
>>  case GlobalValue::ExternalLinkage:
>> +  case GlobalValue::LinkerWeakLinkage:
> 
> Xcore doesn't support this linkage type.  The three darwin targets do support it.  Testcases?  We also need a testcase for .ll file assembly/disassembly.
> 
There's already one testcase in the test suite. However, it has to wait for a future patch before I can convert it to using this schema.

-bw