<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Thanks for reviewing the patch.<br>
<br>
On 2/22/2015 9:20 PM, Rui Ueyama wrote:<br>
</div>
<blockquote
cite="mid:CAJENXgvHpPbQZm2FHJNBJui4PhQKbnEt4S_W2h5su5yQi_WD+Q@mail.gmail.com"
type="cite"><br>
<pre wrap="">
Scope scope() const override {
+ if (!_symbol)
+ return scopeGlobal;
</pre>
<pre wrap="">
How can this condition happen? Looks like previously _symbol was guaranteed
to be non-null. Now, it looks like a null pointer can be assigned to
_symbol so you need null check everywhere.
</pre>
</blockquote>
This is used by the atom thats used for holding the group signature
atom and the gnu linkonce signature atom.<br>
<br>
This is used only by by functions which return properties of the
symbol, which is already taken care in the patch.<br>
<blockquote
cite="mid:CAJENXgvHpPbQZm2FHJNBJui4PhQKbnEt4S_W2h5su5yQi_WD+Q@mail.gmail.com"
type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap=""> if (_symbol->getVisibility() == llvm::ELF::STV_HIDDEN)
return scopeLinkageUnit;
else if (_symbol->getBinding() != llvm::ELF::STB_LOCAL)
@@ -199,6 +201,9 @@ public:
Interposable interposable() const override { return interposeNo; }
Merge merge() const override {
+ if (!_symbol)
+ return mergeNo;
+
if (_symbol->getBinding() == llvm::ELF::STB_WEAK)
return mergeAsWeak;
@@ -216,6 +221,9 @@ public:
ContentType ret = typeUnknown;
uint64_t flags = _section->sh_flags;
+ if (!_symbol && _sectionName.startswith(".gnu.linkonce"))
</pre>
</blockquote>
<pre wrap="">
In the commit message you wrote that sections starts with "gnu.linkonce."
is GNU linkonce sections. You dropped the last "." (dot) here.
</pre>
</blockquote>
Good catch, I will fix this.<br>
<blockquote
cite="mid:CAJENXgvHpPbQZm2FHJNBJui4PhQKbnEt4S_W2h5su5yQi_WD+Q@mail.gmail.com"
type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">+ return typeGnuLinkOnce;
+
if (!(flags & llvm::ELF::SHF_ALLOC))
return _contentType = typeNoAlloc;
@@ -286,6 +294,9 @@ public:
}
Alignment alignment() const override {
+ if (!_symbol)
+ return Alignment(0);
+
// Obtain proper value of st_value field.
const auto symValue = getSymbolValue(_symbol);
@@ -323,7 +334,7 @@ public:
StringRef customSectionName() const override {
if ((contentType() == typeZeroFill) ||
- (_symbol->st_shndx == llvm::ELF::SHN_COMMON))
+ (_symbol && _symbol->st_shndx == llvm::ELF::SHN_COMMON))
return ".bss";
return _sectionName;
}
Modified: lld/trunk/lib/ReaderWriter/ELF/ELFFile.h
URL:
<a class="moz-txt-link-freetext" href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/ELFFile.h?rev=230194&r1=230193&r2=230194&view=diff">http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/ELF/ELFFile.h?rev=230194&r1=230193&r2=230194&view=diff</a>
==============================================================================
--- lld/trunk/lib/ReaderWriter/ELF/ELFFile.h (original)
+++ lld/trunk/lib/ReaderWriter/ELF/ELFFile.h Sun Feb 22 18:04:49 2015
@@ -168,8 +168,23 @@ public:
return _absoluteAtoms;
}
- Atom *findAtom(const Elf_Sym *symbol) {
- return _symbolToAtomMapping.lookup(symbol);
+ Atom *findAtom(const Elf_Sym *sourceSymbol, const Elf_Sym
*targetSymbol) {
+ // All references to atoms inside a group are through undefined atoms.
+ Atom *targetAtom = _symbolToAtomMapping.lookup(targetSymbol);
+ if (targetAtom->definition() != Atom::definitionRegular)
+ return targetAtom;
+ if ((llvm::dyn_cast<DefinedAtom>(targetAtom))->scope() ==
+ DefinedAtom::scopeTranslationUnit)
+ return targetAtom;
+ if (!redirectReferenceUsingUndefAtom(sourceSymbol, targetSymbol))
+ return targetAtom;
+ auto undefForGroupchild =
_undefAtomsForgroupChild.find(targetAtom->name());
+ if (undefForGroupchild != _undefAtomsForgroupChild.end())
+ return undefForGroupchild->getValue();
+ auto undefGroupChildAtom =
+ new (_readerStorage) SimpleUndefinedAtom(*this,
targetAtom->name());
+ _undefinedAtoms._atoms.push_back(undefGroupChildAtom);
+ return (_undefAtomsForgroupChild[targetAtom->name()] =
undefGroupChildAtom);
}
</pre>
</blockquote>
<pre wrap="">
This really needs a comment to describe what the intention of the
conditions is.</pre>
</blockquote>
Sure, I will point to the COMDAT spec listed outside the ELF spec,
as the ELF spec is so out of date.<br>
<blockquote
cite="mid:CAJENXgvHpPbQZm2FHJNBJui4PhQKbnEt4S_W2h5su5yQi_WD+Q@mail.gmail.com"
type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">
protected:
@@ -258,6 +273,12 @@ protected:
return shdr && (shdr->sh_type == llvm::ELF::SHT_PROGBITS) &&
syms.empty();
}
+ /// Handle creation of atoms for .gnu.linkonce sections.
+ std::error_code handleGnuLinkOnceSection(
+ StringRef sectionName,
+ llvm::StringMap<std::vector<ELFDefinedAtom<ELFT> *>>
&atomsForSection,
+ const Elf_Shdr *shdr);
+
/// Process the Undefined symbol and create an atom for it.
ErrorOr<ELFUndefinedAtom<ELFT> *>
handleUndefinedSymbol(StringRef symName, const Elf_Sym *sym) {
@@ -286,6 +307,11 @@ protected:
symbol->st_shndx == llvm::ELF::SHN_COMMON;
}
+ /// Returns true if the section is a gnulinkonce section.
+ bool isGnuLinkOnceSection(StringRef sectionName) const {
+ return sectionName.startswith(".gnu.linkonce");
+ }
+
/// Returns correct st_value for the symbol depending on the
architecture.
/// For most architectures it's just a regular st_value with no changes.
virtual uint64_t getSymbolValue(const Elf_Sym *symbol) const {
@@ -333,6 +359,10 @@ protected:
return mergeAtom;
}
+ /// Does the atom need to be redirected using a separate undefined atom
?
</pre>
</blockquote>
<pre wrap="">
Please remove the space before "?"
Why do we sometimes want to redirect a symbol to an undefined atom? Needs
an explanation here.</pre>
</blockquote>
I will add comment from the COMDAT document. References to functions
<b>from </b>atoms outside the group need to go through an undefined
reference.<br>
<blockquote
cite="mid:CAJENXgvHpPbQZm2FHJNBJui4PhQKbnEt4S_W2h5su5yQi_WD+Q@mail.gmail.com"
type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">+ bool redirectReferenceUsingUndefAtom(const Elf_Sym *sourceSymbol,
+ const Elf_Sym *targetSymbol) const;
+
llvm::BumpPtrAllocator _readerStorage;
std::unique_ptr<llvm::object::ELFFile<ELFT> > _objFile;
atom_collection_vector<DefinedAtom> _definedAtoms;
@@ -350,6 +380,11 @@ protected:
std::unordered_map<StringRef, range<Elf_Rel_Iter>>
_relocationReferences;
std::vector<ELFReference<ELFT> *> _references;
llvm::DenseMap<const Elf_Sym *, Atom *> _symbolToAtomMapping;
+ // Group child atoms have a pair corresponding to the signature and the
+ // section header of the section that was used for generating the
signature.
+ llvm::DenseMap<const Elf_Sym *, std::pair<StringRef, const Elf_Shdr *>>
+ _groupChild;
+ llvm::StringMap<Atom *> _undefAtomsForgroupChild;
</pre>
</blockquote>
<pre wrap="">
undefAtomsForgroupChild -> undefAtomsForGroupChild</pre>
</blockquote>
Sure.<br>
<blockquote
cite="mid:CAJENXgvHpPbQZm2FHJNBJui4PhQKbnEt4S_W2h5su5yQi_WD+Q@mail.gmail.com"
type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap=""> /// \brief Atoms that are created for a section that has the merge
property
/// set
@@ -623,6 +658,11 @@ std::error_code ELFFile<ELFT>::createSym
}
template <class ELFT> std::error_code ELFFile<ELFT>::createAtoms() {
+ // Holds all the atoms that are part of the section. They are the
targets of
+ // the kindGroupChild reference.
+ llvm::StringMap<std::vector<ELFDefinedAtom<ELFT> *>> atomsForSection;
+ // group sections have a mapping of the section header to the signature.
+ llvm::DenseMap<const Elf_Shdr *, StringRef> groupSections;
for (auto &i : _sectionSymbols) {
const Elf_Shdr *section = i.first;
std::vector<Elf_Sym_Iter> &symbols = i.second;
@@ -641,11 +681,21 @@ template <class ELFT> std::error_code EL
if (std::error_code ec = sectionContents.getError())
return ec;
+ bool addAtoms = true;
+
+ if (isGnuLinkOnceSection(*sectionName)) {
+ groupSections.insert(std::make_pair(section, *sectionName));
+ addAtoms = false;
+ }
+
if (handleSectionWithNoSymbols(section, symbols)) {
ELFDefinedAtom<ELFT> *newAtom =
createSectionAtom(section, *sectionName, *sectionContents);
- _definedAtoms._atoms.push_back(newAtom);
newAtom->setOrdinal(++_ordinal);
+ if (addAtoms)
+ _definedAtoms._atoms.push_back(newAtom);
+ else
+ atomsForSection[*sectionName].push_back(newAtom);
continue;
}
@@ -693,8 +743,11 @@ template <class ELFT> std::error_code EL
auto definedMergeAtom = handleDefinedSymbol(
symbolName, *sectionName, &**si, section, symbolData,
_references.size(), _references.size(), _references);
- _definedAtoms._atoms.push_back(*definedMergeAtom);
(*definedMergeAtom)->setOrdinal(++_ordinal);
+ if (addAtoms)
+ _definedAtoms._atoms.push_back(*definedMergeAtom);
+ else
+ atomsForSection[*sectionName].push_back(*definedMergeAtom);
}
continue;
}
@@ -740,19 +793,60 @@ template <class ELFT> std::error_code EL
// is a weak atom.
previousAtom = anonAtom ? anonAtom : newAtom;
- _definedAtoms._atoms.push_back(newAtom);
+ if (addAtoms)
+ _definedAtoms._atoms.push_back(newAtom);
+ else
+ atomsForSection[*sectionName].push_back(newAtom);
+
_symbolToAtomMapping.insert(std::make_pair(&*symbol, newAtom));
if (anonAtom) {
anonAtom->setOrdinal(++_ordinal);
- _definedAtoms._atoms.push_back(anonAtom);
+ if (addAtoms)
+ _definedAtoms._atoms.push_back(anonAtom);
+ else
+ atomsForSection[*sectionName].push_back(anonAtom);
}
}
}
+ // Iterate over all the group sections to create parent atoms pointing
to
+ // group-child atoms.
+ for (auto § : groupSections) {
+ StringRef signature = sect.second;
+ if (isGnuLinkOnceSection(signature))
+ handleGnuLinkOnceSection(signature, atomsForSection, sect.first);
+ }
+
updateReferences();
return std::error_code();
}
+template <class ELFT>
+std::error_code ELFFile<ELFT>::handleGnuLinkOnceSection(
</pre>
</blockquote>
<pre wrap="">
This function never fails, so it should return void isntead of error_code.</pre>
</blockquote>
I need to add error checks to check for an invalid ELF symbol. I
will go ahead and return errors, but its so difficult to create test
cases using ELF object files.<br>
<br>
</body>
</html>