[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
clattner at apple.com
Tue Jun 29 14:56:26 PDT 2010
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?
> +++ 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.
> + <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-c/Core.h Tue Jun 29 16:24:00 2010
> @@ -226,7 +226,8 @@
> LLVMExternalWeakLinkage,/**< ExternalWeak linkage description */
> LLVMGhostLinkage, /**< Obsolete */
> LLVMCommonLinkage, /**< Tentative definitions */
> - LLVMLinkerPrivateLinkage /**< Like Private, but linker removes. */
> + LLVMLinkerPrivateLinkage, /**< Like private, but linker removes. */
> + LLVMLinkerWeakLinkage /**< Like linker private, but weak. */
This enum should also be updated with the rename.
> +++ 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? Please rename it to be more descriptive or improve the comment. "isLinkerPrivate" seems like a perfectly good and descriptive name to me.
> +++ llvm/trunk/include/llvm/GlobalValue.h Tue Jun 29 16:24:00 2010
> @@ -39,7 +39,8 @@
> AppendingLinkage, ///< Special purpose, only applies to global arrays
> InternalLinkage, ///< Rename collisions when linking (static functions).
> PrivateLinkage, ///< Like Internal, but omit from symbol table.
> - LinkerPrivateLinkage, ///< Like Private, but linker removes.
> + LinkerPrivateLinkage, ///< Like private, but linker removes.
> + LinkerWeakLinkage, ///< Like linker private, but weak.
This enum should be renamed.
> DLLImportLinkage, ///< Function to be imported from DLL
> DLLExportLinkage, ///< Function to be accessible from DLL.
> ExternalWeakLinkage,///< ExternalWeak linkage description.
> @@ -132,7 +133,10 @@
> return Linkage == PrivateLinkage;
> }
> static bool isLinkerPrivateLinkage(LinkageTypes Linkage) {
> - return Linkage==LinkerPrivateLinkage;
> + return Linkage == LinkerPrivateLinkage;
> + }
> + static bool isLinkerWeakLinkage(LinkageTypes Linkage) {
> + return Linkage == LinkerWeakLinkage;
> }
> static bool isLocalLinkage(LinkageTypes Linkage) {
> return isInternalLinkage(Linkage) || isPrivateLinkage(Linkage) ||
The predicates should be updated. isLocalLinkage and mayBeOverridden should return true, for example, any others?
> 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?
>
> +++ 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.
> +++ 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.
> +++ 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! :)
>
> 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/CppBackend/CPPBackend.cpp Tue Jun 29 16:24:00 2010
> @@ -286,6 +286,8 @@
> Out << "GlobalValue::PrivateLinkage"; break;
> case GlobalValue::LinkerPrivateLinkage:
> Out << "GlobalValue::LinkerPrivateLinkage"; break;
> + case GlobalValue::LinkerWeakLinkage:
> + Out << "GlobalValue::LinkerWeakLinkage"; break;
This will also need to be renamed.
> +++ llvm/trunk/lib/Target/Mangler.cpp Tue Jun 29 16:24:00 2010
> @@ -118,6 +118,9 @@
> } else if (PrefixTy == Mangler::LinkerPrivate) {
> const char *Prefix = MAI.getLinkerPrivateGlobalPrefix();
> OutName.append(Prefix, Prefix+strlen(Prefix));
> + } else if (PrefixTy == Mangler::LinkerWeak) {
> + const char *Prefix = MAI.getLinkerWeakGlobalPrefix();
> + OutName.append(Prefix, Prefix+strlen(Prefix));
> }
>
> const char *Prefix = MAI.getGlobalPrefix();
> @@ -182,6 +185,8 @@
> PrefixTy = Mangler::Private;
> else if (GV->hasLinkerPrivateLinkage())
> PrefixTy = Mangler::LinkerPrivate;
> + else if (GV->hasLinkerWeakLinkage())
> + PrefixTy = Mangler::LinkerWeak;
These should go away, just mangle both linker private's the same way.
> +++ 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.
-Chris
More information about the llvm-commits
mailing list