[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/

[Chris Lattner]

On Jun 29, 2010, at 3:33 PM, Bill Wendling wrote:

> 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".

They are "as private" as other "l" labels, right?  Those are visible across translation units too.  We call it "linker private" so calling this "linker_private_weak" seems reasonable.  Any other suggestions?

>>> 
>>> 
>>>   /// 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.

It seems that a preferred fix is to pass in the Mangler enum, which is really what this function needs.  Alternatively (if that would require adding a #include to a header), split it into two different functions, one that is private and one that is linker private).

>> 
>> 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.

I don't really understand what you mean.

>>> 
>>> +/// 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?

This looks like code reformatting only.

Thanks for working on this Bill!

-Chris