[LTO] Prevent dead stripping and internalization of symbols with sections

[tejohnson]

Summary

ELF linkers generate start_<secname> and stop_<secname> symbols
when there is a value in a section <secname> where the name is a valid
C identifier. If dead stripping determines that the values declared
in section <secname> are dead, and we then internalize (and delete)
such a symbol, programs that reference the corresponding start and end
section symbols will get undefined reference linking errors.

To fix this, add a flag to the IRSymtab to track when a symbol is
defined in an ELF object and has a C identifier section name. Then
use this in LTO to mark the symbol as external and visible from outside
the summary.

[davide]

Do you know whether LTO gets this case right?

[tejohnson]

In D35639#815079, @davide wrote:

Do you know whether LTO gets this case right?

It does not get it correct. I could move the fix into the internalize handling under llvm::internalizeModule: that would cover ThinLTO with the new LTO API, and ThinLTO and regular LTO with the legacy LTO API. However, the regular LTO handling in LTO::runRegularLTO does not use internalizeModule. I would either need to duplicate the fix for regular LTO into both the new and legacy LTO handling, or move the fix into internalizeModule and refactor runRegularLTO to use it as well. WDYT?

[davide]

In D35639#815191, @tejohnson wrote:

In D35639#815079, @davide wrote:

Do you know whether LTO gets this case right?

It does not get it correct. I could move the fix into the internalize handling under llvm::internalizeModule: that would cover ThinLTO with the new LTO API, and ThinLTO and regular LTO with the legacy LTO API. However, the regular LTO handling in LTO::runRegularLTO does not use internalizeModule. I would either need to duplicate the fix for regular LTO into both the new and legacy LTO handling, or move the fix into internalizeModule and refactor runRegularLTO to use it as well. WDYT?

I'm not sure. This is actually exposing a bigger problem that we need to solve sooner rather than later (i.e. the fact there are two APIs)
Probably we can duplicate the fix for the time being but we should also have a discussion about it.

[pcc]

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

[davide]

In D35639#815234, @pcc wrote:

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

That makes sense to me. This kind of goes against the principles of the API as we're going to duplicate this at least between lld and gold, but I don't think it's unreasonable.
In particular, this seems ELF-specific so other formats might not really care, therefore the client seems to be the most obvious choice.

[tejohnson]

In D35639#815208, @davide wrote:

In D35639#815191, @tejohnson wrote:

In D35639#815079, @davide wrote:

Do you know whether LTO gets this case right?

It does not get it correct. I could move the fix into the internalize handling under llvm::internalizeModule: that would cover ThinLTO with the new LTO API, and ThinLTO and regular LTO with the legacy LTO API. However, the regular LTO handling in LTO::runRegularLTO does not use internalizeModule. I would either need to duplicate the fix for regular LTO into both the new and legacy LTO handling, or move the fix into internalizeModule and refactor runRegularLTO to use it as well. WDYT?

I'm not sure. This is actually exposing a bigger problem that we need to solve sooner rather than later (i.e. the fact there are two APIs)
Probably we can duplicate the fix for the time being but we should also have a discussion about it.

Ironically, it is not the different APIs that are an issue - it is the different internalization handling for regular vs Thin LTO in the new API (the former updates the linkage directly, the latter invokes internalizeModule), that would provoke duplication (unless I refactored the former to also use internalizeModule).

[tejohnson]

In D35639#815234, @pcc wrote:

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

I don't see how to get the presence of a section or the section name for a symbol when walking the symbols? I.e. either in gold-plugin via the symbol info from the linker, or in LTO::addModuleToGlobalRes via the InputFile:Symbol?

[pcc]

In D35639#815578, @tejohnson wrote:

In D35639#815234, @pcc wrote:

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

I don't see how to get the presence of a section or the section name for a symbol when walking the symbols? I.e. either in gold-plugin via the symbol info from the linker, or in LTO::addModuleToGlobalRes via the InputFile:Symbol?

We'd need to add that information to the irsymtab and expose it via InputFile::Symbol.

[tejohnson]

In D35639#815579, @pcc wrote:

In D35639#815578, @tejohnson wrote:

In D35639#815234, @pcc wrote:

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

I don't see how to get the presence of a section or the section name for a symbol when walking the symbols? I.e. either in gold-plugin via the symbol info from the linker, or in LTO::addModuleToGlobalRes via the InputFile:Symbol?

We'd need to add that information to the irsymtab and expose it via InputFile::Symbol.

That's what I was afraid of. =( That seems quite a bit more extensive than simply checking whether the GV has a section and blocking internalization.
If we were to go the route you are suggesting, it looks like currently we don't iterate through the InputFile until we invoke the LTO API, when adding a module. I guess we would have to then iterate through those and update the global resolutions earlier, e.g. in the gold-plugin? I.e. in order to make it ELF specific.

[pcc]

In D35639#815587, @tejohnson wrote:

In D35639#815579, @pcc wrote:

In D35639#815578, @tejohnson wrote:

In D35639#815234, @pcc wrote:

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

I don't see how to get the presence of a section or the section name for a symbol when walking the symbols? I.e. either in gold-plugin via the symbol info from the linker, or in LTO::addModuleToGlobalRes via the InputFile:Symbol?

We'd need to add that information to the irsymtab and expose it via InputFile::Symbol.

That's what I was afraid of. =( That seems quite a bit more extensive than simply checking whether the GV has a section and blocking internalization.
If we were to go the route you are suggesting, it looks like currently we don't iterate through the InputFile until we invoke the LTO API, when adding a module. I guess we would have to then iterate through those and update the global resolutions earlier, e.g. in the gold-plugin? I.e. in order to make it ELF specific.

I don't follow your point about global resolutions, this should be purely a local analysis (from the linker's perspective at least). In both the gold plugin and lld an InputFile is available at the point where we compute the resolution for each symbol, so it should be a matter of copying the property from the InputFile symbol to the resolution.

[tejohnson]

In D35639#815591, @pcc wrote:

In D35639#815587, @tejohnson wrote:

In D35639#815579, @pcc wrote:

In D35639#815578, @tejohnson wrote:

In D35639#815234, @pcc wrote:

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

I don't see how to get the presence of a section or the section name for a symbol when walking the symbols? I.e. either in gold-plugin via the symbol info from the linker, or in LTO::addModuleToGlobalRes via the InputFile:Symbol?

We'd need to add that information to the irsymtab and expose it via InputFile::Symbol.

That's what I was afraid of. =( That seems quite a bit more extensive than simply checking whether the GV has a section and blocking internalization.
If we were to go the route you are suggesting, it looks like currently we don't iterate through the InputFile until we invoke the LTO API, when adding a module. I guess we would have to then iterate through those and update the global resolutions earlier, e.g. in the gold-plugin? I.e. in order to make it ELF specific.

I don't follow your point about global resolutions, this should be purely a local analysis (from the linker's perspective at least). In both the gold plugin and lld an InputFile is available at the point where we compute the resolution for each symbol, so it should be a matter of copying the property from the InputFile symbol to the resolution.

Yeah I meant the SymbolResolution not the GlobalResolution.

[tejohnson]

In D35639#815632, @tejohnson wrote:

In D35639#815591, @pcc wrote:

In D35639#815587, @tejohnson wrote:

In D35639#815579, @pcc wrote:

In D35639#815578, @tejohnson wrote:

In D35639#815234, @pcc wrote:

Another possibility is to handle this in the client (i.e. the linker) instead. The client has the best idea of whether a given section is a GC root; ELF linkers can set VisibleToRegularObj on any symbol in a section named like a C identifier, and other linkers can do whatever is appropriate for their object format.

I don't see how to get the presence of a section or the section name for a symbol when walking the symbols? I.e. either in gold-plugin via the symbol info from the linker, or in LTO::addModuleToGlobalRes via the InputFile:Symbol?

We'd need to add that information to the irsymtab and expose it via InputFile::Symbol.

That's what I was afraid of. =( That seems quite a bit more extensive than simply checking whether the GV has a section and blocking internalization.
If we were to go the route you are suggesting, it looks like currently we don't iterate through the InputFile until we invoke the LTO API, when adding a module. I guess we would have to then iterate through those and update the global resolutions earlier, e.g. in the gold-plugin? I.e. in order to make it ELF specific.

I don't follow your point about global resolutions, this should be purely a local analysis (from the linker's perspective at least). In both the gold plugin and lld an InputFile is available at the point where we compute the resolution for each symbol, so it should be a matter of copying the property from the InputFile symbol to the resolution.

Yeah I meant the SymbolResolution not the GlobalResolution.

Actually, I have a variant of this that I prefer. Instead of setting VisibleToRegularObj in each linker, we could handle it similar to the way some COFF specific behavior is handled. I.e. when building the symbol table, if isOSBinFormatELF, and the symbol has a C identifier section name, then set a (new) flag on the symbol indicating as such (e.g. ELFHasCIdentifierSectionName). Then in LTO::addModuleToGlobalRes, we add a check of this flag to the conditions under which VisibleOutsideSummary is set. That has the advantage of avoiding the need to add checking to each ELF linker, and also doesn't overload the VisibleToRegularObj flag with a condition that isn't being visible to a regular object, but rather visible outside the summary in a different way. Sound ok?

[tejohnson]

In D35639#816132, @tejohnson wrote:

Actually, I have a variant of this that I prefer. Instead of setting VisibleToRegularObj in each linker, we could handle it similar to the way some COFF specific behavior is handled. I.e. when building the symbol table, if isOSBinFormatELF, and the symbol has a C identifier section name, then set a (new) flag on the symbol indicating as such (e.g. ELFHasCIdentifierSectionName). Then in LTO::addModuleToGlobalRes, we add a check of this flag to the conditions under which VisibleOutsideSummary is set. That has the advantage of avoiding the need to add checking to each ELF linker, and also doesn't overload the VisibleToRegularObj flag with a condition that isn't being visible to a regular object, but rather visible outside the summary in a different way. Sound ok?

Note in addModuleToGlobalRes it is also added to the set of conditions where we need to set the Partition to External (similar to the places where we currently check things like VisibleToRegularObj. I have this working for a small test case, testing with my full internal test case. If that works I will clean up and send for review. I like this since it is ELF specific but doesn't require any linker changes.

[pcc]

In D35639#816341, @tejohnson wrote:

In D35639#816132, @tejohnson wrote:

Actually, I have a variant of this that I prefer. Instead of setting VisibleToRegularObj in each linker, we could handle it similar to the way some COFF specific behavior is handled. I.e. when building the symbol table, if isOSBinFormatELF, and the symbol has a C identifier section name, then set a (new) flag on the symbol indicating as such (e.g. ELFHasCIdentifierSectionName). Then in LTO::addModuleToGlobalRes, we add a check of this flag to the conditions under which VisibleOutsideSummary is set. That has the advantage of avoiding the need to add checking to each ELF linker, and also doesn't overload the VisibleToRegularObj flag with a condition that isn't being visible to a regular object, but rather visible outside the summary in a different way. Sound ok?

Note in addModuleToGlobalRes it is also added to the set of conditions where we need to set the Partition to External (similar to the places where we currently check things like VisibleToRegularObj. I have this working for a small test case, testing with my full internal test case. If that works I will clean up and send for review. I like this since it is ELF specific but doesn't require any linker changes.

Sounds fine to me.

Note that this design would preclude the linker from doing anything smarter with VisibleToRegularObj, like setting it depending on whether the input files contain an undefined reference to the __start_ or __stop_ symbols. We may also need section names in order to properly support certain linker script features with LTO. But I guess we can always change the design if any of that turns out to be necessary/useful.

[tejohnson]

Address comments.

[mehdi_amini]

lib/LTO/LTO.cpp

391 ↗

(On Diff #107579)

I don't understand why this is the right place to do this? Why isn't the responsibility of the linker resolution to mark these symbols as externally visible?

[tejohnson]

lib/LTO/LTO.cpp

391 ↗

(On Diff #107579)

The start_ and end_ symbols for these sections are implicitly generated by the linker. At least in the gold plugin, we may never see the references to these special symbols (e.g. if they were in a regular object not even processed by the plugin). So the symbols that are declared in such sections (provoking the linker's generation of these symbols) need to be treated conservatively. Doing it here covers all ELF linkers.

[mehdi_amini]

lib/LTO/LTO.cpp

391 ↗

(On Diff #107579)

But this is the reason we have a resolution API: we shouldn't have such "magic" here.
If these symbols must be preserved because the linker may add a reference to it later, the responsibility should be on the linker client of the API to pass the right resolution.

[pcc]

lib/LTO/LTO.cpp

391 ↗

(On Diff #107579)

I agree with Mehdi, FWIW. I can see the argument for why one might choose to handle this in the API, but it's not the way that I would implement it.

[mehdi_amini]

lib/LTO/LTO.cpp

391 ↗

(On Diff #107579)

I just thought about an analogy with another piece of magic: we prevent renaming of symbols that have a section name attached. This is also some "magic" (that I added) that isn't in the right place. However it can't be addressed by the "resolution" API, it would need a new construct for the linker / client of the API to express the fact that a symbol can't be renamed.

[tejohnson]

Address review comments, moving handling to gold-plugin (I'd like to get
this one in asap and add the lld support in a follow up).

[pcc]

s/__end_/__stop_/ in a bunch of places.

include/llvm/Object/IRSymtab.h

130

I'd prefer to put the entire section name in here so that we can use it for the things I mentioned earlier.

Otherwise this can be moved to Symbol::Flags, there's plenty of space there.

test/tools/gold/X86/global_with_section.ll

11

Is the "-u foo" part necessary? Same below.

46

Probably also want a negative test that shows we don't do this to functions with a section name that isn't a C identifier.

tools/gold/gold-plugin.cpp

624

Probably a little more precise to do assert(InputFileSyms.size() == F.syms.size()) outside of the loop instead.

659–666

I'd move the section name check here outside the switch statement.

tools/llvm-lto2/llvm-lto2.cpp

360

identifier

[tejohnson]

include/llvm/Object/IRSymtab.h

130

Changed to track section name.

test/tools/gold/X86/global_with_section.ll

11

Turns out no, so I removed foo. This was a carry over from when I was testing the LTO-only version of the fix, using internal tools that expected to have an exported symbol.

[tejohnson]

Address review comments

[pcc]

lib/Object/IRSymtab.cpp

256

I wouldn't check whether the section name is a C identifier here, as that is the client's responsibility. That probably means that the field should be renamed to just SectionName.

[tejohnson]

Address comments

[pcc]

LGTM

include/llvm/LTO/LTO.h

public:
     using irsymtab::Symbol::getName;
     using irsymtab::Symbol::getVisibility;
     using irsymtab::Symbol::canBeOmittedFromSymbolTable;
     using irsymtab::Symbol::isTLS;
     using irsymtab::Symbol::getComdatIndex;
     using irsymtab::Symbol::getCommonSize;
     using irsymtab::Symbol::getCommonAlignment;
     using irsymtab::Symbol::getCOFFWeakExternalFallback;
+    using irsymtab::Symbol::getELFCIdentifierSectionName;
     using irsymtab::Symbol::isExecutable;
   };
 
   /// A range over the symbols in this InputFile.
   ArrayRef<Symbol> symbols() const { return Symbols; }
 
   /// Returns linker options specified in the input file.
   StringRef getCOFFLinkerOpts() const { return COFFLinkerOpts; }

include/llvm/Object/IRSymtab.h

 /// This data structure contains rarely used symbol fields and is optionally
 /// referenced by a Symbol.
 struct Uncommon {
   Word CommonSize, CommonAlign;
 
   /// COFF-specific: the name of the symbol that a weak external resolves to
   /// if not defined.
   Str COFFWeakExternFallbackName;
+
+  /// ELF-specific: ELF linkers generate __start_<secname> and __stop_<secname>
+  /// symbols when there is a value in a section <secname> where the name is a
+  /// valid C identifier. Track symbols in such a section, so that we can
+  /// ensure that they aren't internalized and eliminated, which would
+  /// suppress the generation of the special __start_ and __stop_ symbols.
+  Str ELFCIdentifierSectionName;
 };
 
 struct Header {
   /// Version number of the symtab format. This number should be incremented
   /// when the format changes, but it does not need to be incremented if a
   /// change to LLVM would cause it to create a different symbol table.
   Word Version;
-  enum { kCurrentVersion = 0 };
+  enum { kCurrentVersion = 1 };
 
   /// The producer's version string (LLVM_VERSION_STRING " " LLVM_REVISION).
   /// Consumers should rebuild the symbol table from IR if the producer's
   /// version does not match the consumer's version due to potential differences
   /// in symbol table format, symbol enumeration order and so on.
   Str Producer;
 
   Range<Module> Modules;
struct Symbol {
   // Copied from storage::Symbol.
   StringRef Name, IRName;
   int ComdatIndex;
   uint32_t Flags;
 
   // Copied from storage::Uncommon.
   uint32_t CommonSize, CommonAlign;
   StringRef COFFWeakExternFallbackName;
+  StringRef ELFCIdentifierSectionName;
 
   /// Returns the mangled symbol name.
   StringRef getName() const { return Name; }
 
   /// Returns the unmangled symbol name, or the empty string if this is not an
   /// IR symbol.
   StringRef getIRName() const { return IRName; }
 
struct Symbol {
   }
 
   /// COFF-specific: for weak externals, returns the name of the symbol that is
   /// used as a fallback if the weak external remains undefined.
   StringRef getCOFFWeakExternalFallback() const {
     assert(isWeak() && isIndirect());
     return COFFWeakExternFallbackName;
   }
+
+  /// ELF-specific: ELF linkers generate __start_<secname> and __stop_<secname>
+  /// symbols when there is a value in a section <secname> where the name is a
+  /// valid C identifier. Track symbols in such a section, so that we can
+  /// ensure that they aren't internalized and eliminated, which would
+  /// suppress the generation of the special __start_ and __stop_ symbols.
+  StringRef getELFCIdentifierSectionName() const {
+    return ELFCIdentifierSectionName;
+  }
 };
 
 /// This class can be used to read a Symtab and Strtab produced by
 /// irsymtab::build.
 class Reader {
   StringRef Symtab, Strtab;
 
   ArrayRef<storage::Module> Modules;
void read() {
     IRName = R->str(SymI->IRName);
     ComdatIndex = SymI->ComdatIndex;
     Flags = SymI->Flags;
 
     if (Flags & (1 << storage::Symbol::FB_has_uncommon)) {
       CommonSize = UncI->CommonSize;
       CommonAlign = UncI->CommonAlign;
       COFFWeakExternFallbackName = R->str(UncI->COFFWeakExternFallbackName);
-    }
+      ELFCIdentifierSectionName = R->str(UncI->ELFCIdentifierSectionName);
+    } else
+      // Reset this field so it can be queried unconditionally for all symbols.
+      ELFCIdentifierSectionName = "";
   }
 
 public:
   SymbolRef(const storage::Symbol *SymI, const storage::Symbol *SymE,
             const storage::Uncommon *UncI, const Reader *R)
       : SymI(SymI), SymE(SymE), UncI(UncI), R(R) {
     read();
   }

lib/Object/IRSymtab.cpp

Error Builder::addModule(Module *M) {
 
   for (ModuleSymbolTable::Symbol Msym : Msymtab.symbols())
     if (Error Err = addSymbol(Msymtab, Used, Msym))
       return Err;
 
   return Error::success();
 }
 
+static bool isAlpha(char C) {
+  return ('a' <= C && C <= 'z') || ('A' <= C && C <= 'Z') || C == '_';
+}
+
+static bool isAlnum(char C) { return isAlpha(C) || ('0' <= C && C <= '9'); }
+
+// Returns true if S is valid as a C language identifier.
+static bool isValidCIdentifier(StringRef S) {
+  return !S.empty() && isAlpha(S[0]) &&
+         std::all_of(S.begin() + 1, S.end(), isAlnum);
+}
+
 Error Builder::addSymbol(const ModuleSymbolTable &Msymtab,
                          const SmallPtrSet<GlobalValue *, 8> &Used,
                          ModuleSymbolTable::Symbol Msym) {
   Syms.emplace_back();
   storage::Symbol &Sym = Syms.back();
   Sym = {};
 
   storage::Uncommon *Unc = nullptr;
   auto Uncommon = [&]() -> storage::Uncommon & {
     if (Unc)
       return *Unc;
     Sym.Flags |= 1 << storage::Symbol::FB_has_uncommon;
     Uncommons.emplace_back();
     Unc = &Uncommons.back();
     *Unc = {};
     setStr(Unc->COFFWeakExternFallbackName, "");
+    setStr(Unc->ELFCIdentifierSectionName, "");
     return *Unc;
   };
 
   SmallString<64> Name;
   {
     raw_svector_ostream OS(Name);
     Msymtab.printSymbolName(OS, Msym);
   }
if ((Flags & object::BasicSymbolRef::SF_Weak) &&
       Msymtab.printSymbolName(
           OS, cast<GlobalValue>(
                   cast<GlobalAlias>(GV)->getAliasee()->stripPointerCasts()));
       OS.flush();
       setStr(Uncommon().COFFWeakExternFallbackName, Saver.save(FallbackName));
     }
   }
 
+  if (TT.isOSBinFormatELF() && isValidCIdentifier(Base->getSection()))
+    setStr(Uncommon().ELFCIdentifierSectionName,
+           Saver.save(Base->getSection()));
+
   return Error::success();
 }
 
 Error Builder::build(ArrayRef<Module *> IRMods) {
   storage::Header Hdr;
 
   assert(!IRMods.empty());
   Hdr.Version = storage::Header::kCurrentVersion;

test/Object/X86/irsymtab.ll

 ; RUN: env LLVM_OVERRIDE_PRODUCER=producer opt -o %t %s
 ; RUN: llvm-bcanalyzer -dump -show-binary-blobs %t | FileCheck --check-prefix=BCA %s
 
 ; Same producer, does not require upgrade.
 ; RUN: env LLVM_OVERRIDE_PRODUCER=producer llvm-lto2 dump-symtab %t | FileCheck --check-prefix=SYMTAB %s
 
 ; Different producer, requires upgrade.
 ; RUN: env LLVM_OVERRIDE_PRODUCER=consumer llvm-lto2 dump-symtab %t | FileCheck --check-prefix=SYMTAB %s
 
 ; BCA:      <SYMTAB_BLOCK
 ; Version stored at offset 0.
-; BCA-NEXT:   <BLOB abbrevid=4/> blob data = '\x00\x00\x00\x00\x06\x00\x00\x00\x08\x00\x00\x00D\x00\x00\x00\x01\x00\x00\x00P\x00\x00\x00\x00\x00\x00\x00P\x00\x00\x00\x02\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00\x0E\x00\x00\x00\x18\x00\x00\x00&\x00\x00\x00\x0B\x00\x00\x001\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x00$\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x08$\x00\x00'
+; BCA-NEXT:   <BLOB abbrevid=4/> blob data = '\x01\x00\x00\x00\x06\x00\x00\x00\x08\x00\x00\x00D\x00\x00\x00\x01\x00\x00\x00P\x00\x00\x00\x00\x00\x00\x00P\x00\x00\x00\x02\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00\x0E\x00\x00\x00\x18\x00\x00\x00&\x00\x00\x00\x0B\x00\x00\x001\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x00$\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\x03\x00\x00\x00\xFF\xFF\xFF\xFF\x08$\x00\x00'
 ; BCA-NEXT: </SYMTAB_BLOCK>
 ; BCA-NEXT: <STRTAB_BLOCK
 ; BCA-NEXT:   <BLOB abbrevid=4/> blob data = 'foobarproducerx86_64-unknown-linux-gnuirsymtab.ll'
 ; BCA-NEXT: </STRTAB_BLOCK>
 
-; SYMTAB:      version: 0
+; SYMTAB:      version: 1
 ; SYMTAB-NEXT: producer: producer
 ; SYMTAB-NEXT: target triple: x86_64-unknown-linux-gnu
 ; SYMTAB-NEXT: source filename: irsymtab.ll
 ; SYMTAB-NEXT: D------X foo
 ; SYMTAB-NEXT: DU-----X bar
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 source_filename = "irsymtab.ll"
 
 define void @foo() {
   ret void
 }
 
 declare void @bar()

test/tools/gold/X86/Inputs/global_with_section.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @deadfunc2_called_from_section() {
+  ret void
+}
+
+define void @deadfunc2_called_from_nonC_section() {
+  ret void
+}

test/tools/gold/X86/global_with_section.ll

+; Test to ensure we don't internalize or treat as dead a global value
+; with a section. Otherwise, ELF linker generation of __start_"sectionname"
+; and __stop_"sectionname" symbols would not occur and we can end up
+; with undefined references at link time.
+
+; First try RegularLTO
+; RUN: opt %s -o %t.o
+; RUN: llvm-lto2 dump-symtab %t.o | FileCheck %s --check-prefix=SYMTAB
+; RUN: opt %p/Inputs/global_with_section.ll -o %t2.o
+; RUN: %gold -m elf_x86_64 -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:     --plugin-opt=save-temps \
+; RUN:     -o %t3.o %t.o %t2.o
+; Check results of internalization
+; RUN: llvm-dis %t3.o.0.2.internalize.bc -o - | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK2-REGULARLTO
+
+; Next try ThinLTO
+; RUN: opt -module-summary %s -o %t.o
+; RUN: llvm-lto2 dump-symtab %t.o | FileCheck %s --check-prefix=SYMTAB
+; RUN: opt -module-summary %p/Inputs/global_with_section.ll -o %t2.o
+; RUN: %gold -m elf_x86_64 -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:     --plugin-opt=thinlto \
+; RUN:     --plugin-opt=save-temps \
+; RUN:     -o %t3.o %t.o %t2.o
+; Check results of internalization
+; RUN: llvm-dis %t.o.2.internalize.bc -o - | FileCheck %s
+; RUN: llvm-dis %t2.o.2.internalize.bc -o - | FileCheck %s --check-prefix=CHECK2-THINLTO
+
+; SYMTAB: deadfunc_with_section
+; SYMTAB-NEXT: C identifier section name some_other_section
+; SYMTAB-NEXT: deadfunc_with_nonC_section
+; SYMTAB-NEXT: deadfunc2_called_from_section
+; SYMTAB-NEXT: deadfunc2_called_from_nonC_section
+; SYMTAB-NEXT: var_with_section
+; SYMTAB-NEXT: C identifier section name some_section
+; SYMTAB-NEXT: var_with_nonC_section
+; Ensure we don't have a section name for the above symbol
+; SYMTAB-NOT: C identifier section name
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; We should not internalize @var_with_section due to section
+; CHECK-DAG: @var_with_section = global i32 0, section "some_section"
+@var_with_section = global i32 0, section "some_section"
+
+; Confirm via a variable with a non-C identifier section that we are getting
+; the expected internalization.
+; CHECK-DAG: @var_with_nonC_section = internal global i32 0, section ".nonCsection"
+@var_with_nonC_section = global i32 0, section ".nonCsection"
+
+; We should not internalize @deadfunc_with_section due to section
+; CHECK-DAG: define void @deadfunc_with_section() section "some_other_section"
+define void @deadfunc_with_section() section "some_other_section" {
+  call void @deadfunc2_called_from_section()
+  ret void
+}
+
+; Confirm via a function with a non-C identifier section that we are getting
+; the expected internalization.
+; CHECK-DAG: define internal void @deadfunc_with_nonC_section() section ".nonCsection"
+define void @deadfunc_with_nonC_section() section ".nonCsection" {
+  call void @deadfunc2_called_from_nonC_section()
+  ret void
+}
+
+; In RegularLTO mode, where we have combined all the IR,
+; @deadfunc2_called_from_section can be internalized.
+; CHECK2-REGULARLTO: define internal void @deadfunc2_called_from_section
+; In ThinLTO mode, we can't internalize it as it needs to be preserved
+; (due to the access from @deadfunc_with_section which must be preserved), and
+; can't be internalized since the reference is from a different module.
+; CHECK2-THINLTO: define void @deadfunc2_called_from_section
+declare void @deadfunc2_called_from_section()
+
+; Confirm when called from a function with a non-C identifier section that we
+; are getting the expected internalization.
+; CHECK2-REGULARLTO: define internal void @deadfunc2_called_from_nonC_section
+; CHECK2-THINLTO: define internal void @deadfunc2_called_from_nonC_section
+declare void @deadfunc2_called_from_nonC_section()

tools/gold/gold-plugin.cpp

MemoryBufferRef BufferRef(StringRef((const char *)View, F.filesize),
                             Filename);
   Expected<std::unique_ptr<InputFile>> ObjOrErr = InputFile::create(BufferRef);
 
   if (!ObjOrErr)
     message(LDPL_FATAL, "Could not read bitcode from file : %s",
             toString(ObjOrErr.takeError()).c_str());
 
   unsigned SymNum = 0;
+  std::unique_ptr<InputFile> Input = std::move(ObjOrErr.get());
+  auto InputFileSyms = Input->symbols();
+  assert(InputFileSyms.size() == F.syms.size());
   std::vector<SymbolResolution> Resols(F.syms.size());
   for (ld_plugin_symbol &Sym : F.syms) {
+    const InputFile::Symbol &InpSym = InputFileSyms[SymNum];
     SymbolResolution &R = Resols[SymNum++];
 
     ld_plugin_symbol_resolution Resolution =
         (ld_plugin_symbol_resolution)Sym.resolution;
 
     ResolutionInfo &Res = ResInfo[Sym.name];
 
     switch (Resolution) {
case LDPR_PREVAILING_DEF:
       break;
 
     case LDPR_PREVAILING_DEF_IRONLY_EXP:
       R.Prevailing = true;
       if (!Res.CanOmitFromDynSym)
         R.VisibleToRegularObj = true;
       break;
     }
 
+    // If the symbol has a C identifier section name, we need to mark
+    // it as visible to a regular object so that LTO will keep it around
+    // to ensure the linker generates special __start_<secname> and
+    // __stop_<secname> symbols which may be used elsewhere.
+    if (!InpSym.getELFCIdentifierSectionName().empty())
+      R.VisibleToRegularObj = true;
+
     if (Resolution != LDPR_RESOLVED_DYN && Resolution != LDPR_UNDEF &&
         (IsExecutable || !Res.DefaultVisibility))
       R.FinalDefinitionInLinkageUnit = true;
 
     freeSymName(Sym);
   }
 
-  check(Lto.add(std::move(*ObjOrErr), Resols),
+  check(Lto.add(std::move(Input), Resols),
         std::string("Failed to link module ") + F.name);
 }
 
 static void recordFile(std::string Filename, bool TempOutFile) {
   if (add_input_file(Filename.c_str()) != LDPS_OK)
     message(LDPL_FATAL,
             "Unable to add .o file to the link. File left behind in: %s",
             Filename.c_str());

tools/llvm-lto2/llvm-lto2.cpp

for (const InputFile::Symbol &Sym : Input->symbols()) {
                << Sym.getCommonAlignment() << '\n';
 
       int Comdat = Sym.getComdatIndex();
       if (Comdat != -1)
         outs() << "         comdat " << ComdatTable[Comdat] << '\n';
 
       if (TT.isOSBinFormatCOFF() && Sym.isWeak() && Sym.isIndirect())
         outs() << "         fallback " << Sym.getCOFFWeakExternalFallback() << '\n';
+
+      if (TT.isOSBinFormatELF() && !Sym.getELFCIdentifierSectionName().empty())
+        outs() << "         C identifier section name "
+               << Sym.getELFCIdentifierSectionName() << "\n";
     }
 
     outs() << '\n';
   }
 
   return 0;
 }