<div dir="ltr">On Thu, Aug 8, 2013 at 4:31 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Author: ruiu<br>
Date: Thu Aug 8 18:31:50 2013<br>
New Revision: 188025<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=188025&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=188025&view=rev</a><br>
Log:<br>
[PECOFF] Support COMDAT section that contains mergeable atoms.<br>
<br>
The COMDAT section is a section with a special attribute to tell the linker<br>
whether the symbols in the section are allowed to be merged or not. This patch<br>
add a function to interpret the COMDAT data and set "merge" attribute to the<br>
atoms accordingly.<br>
<br>
LLD supports multiple policies to merge atoms; atoms can be merged by name or<br>
by content. COFF supports them, and in addition to that, it supports<br>
choose-the-largest-atom policy, which LLD currently does not support. I simply<br>
mapped it to merge-by-name attribute for now, but we eventually have to support<br>
that policy in the core linker.<br>
<br>
Added:<br>
lld/trunk/test/pecoff/Inputs/comdat.obj.yaml<br>
lld/trunk/test/pecoff/comdat.test<br>
Modified:<br>
lld/trunk/lib/ReaderWriter/PECOFF/Atoms.h<br>
lld/trunk/lib/ReaderWriter/PECOFF/ReaderCOFF.cpp<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/PECOFF/Atoms.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/PECOFF/Atoms.h?rev=188025&r1=188024&r2=188025&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/PECOFF/Atoms.h?rev=188025&r1=188024&r2=188025&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/lib/ReaderWriter/PECOFF/Atoms.h (original)<br>
+++ lld/trunk/lib/ReaderWriter/PECOFF/Atoms.h Thu Aug 8 18:31:50 2013<br>
@@ -175,15 +175,17 @@ class COFFDefinedAtom : public COFFDefin<br>
public:<br>
COFFDefinedAtom(const File &file, StringRef name, StringRef sectionName,<br>
Scope scope, ContentType type, ContentPermissions perms,<br>
- ArrayRef<uint8_t> data, uint64_t ordinal)<br>
+ Merge merge, ArrayRef<uint8_t> data, uint64_t ordinal)<br>
: COFFDefinedFileAtom(file, name, sectionName, scope, type, perms,<br>
ordinal),<br>
- _dataref(data) {}<br>
+ _merge(merge), _dataref(data) {}<br>
<br>
+ virtual Merge merge() const { return _merge; }<br>
virtual uint64_t size() const { return _dataref.size(); }<br>
virtual ArrayRef<uint8_t> rawContent() const { return _dataref; }<br>
<br>
private:<br>
+ Merge _merge;<br>
ArrayRef<uint8_t> _dataref;<br>
};<br>
<br>
<br>
Modified: lld/trunk/lib/ReaderWriter/PECOFF/ReaderCOFF.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/PECOFF/ReaderCOFF.cpp?rev=188025&r1=188024&r2=188025&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/PECOFF/ReaderCOFF.cpp?rev=188025&r1=188024&r2=188025&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/lib/ReaderWriter/PECOFF/ReaderCOFF.cpp (original)<br>
+++ lld/trunk/lib/ReaderWriter/PECOFF/ReaderCOFF.cpp Thu Aug 8 18:31:50 2013<br>
@@ -38,6 +38,7 @@ using lld::coff::COFFDefinedAtom;<br>
using lld::coff::COFFDefinedFileAtom;<br>
using lld::coff::COFFReference;<br>
using lld::coff::COFFUndefinedAtom;<br>
+using llvm::object::coff_aux_section_definition;<br>
using llvm::object::coff_relocation;<br>
using llvm::object::coff_section;<br>
using llvm::object::coff_symbol;<br>
@@ -46,6 +47,7 @@ using namespace lld;<br>
<br>
namespace {<br>
<br>
+// Converts the COFF symbol attribute to the LLD's atom attribute.<br>
Atom::Scope getScope(const coff_symbol *symbol) {<br>
switch (symbol->StorageClass) {<br>
case llvm::COFF::IMAGE_SYM_CLASS_EXTERNAL:<br>
@@ -79,6 +81,16 @@ DefinedAtom::ContentPermissions getPermi<br>
return DefinedAtom::perm___;<br>
}<br>
<br>
+DefinedAtom::Merge getMerge(const coff_aux_section_definition *auxsym) {<br>
+ switch (auxsym->Selection) {<br>
+ case llvm::COFF::IMAGE_COMDAT_SELECT_NODUPLICATES:<br>
+ return DefinedAtom::mergeNo;<br>
+ default:<br>
+ // FIXME: COFF supports more mergable symbol attributes.<br></blockquote><div><br></div><div>Maybe it would be better to list the fully cover the switch and return mergeNo for any case that lld doesn't support yet?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ return DefinedAtom::mergeAsWeakAndAddressUsed;<br>
+ }<br>
+}<br>
+<br>
class FileCOFF : public File {<br>
private:<br>
typedef vector<const coff_symbol *> SymbolVectorT;<br>
@@ -201,6 +213,13 @@ private:<br>
/// we need to know the adjacent symbols.<br>
error_code createDefinedSymbols(const SymbolVectorT &symbols,<br>
vector<const DefinedAtom *> &result) {<br>
+ // A defined atom can be merged if its section attribute allows its contents<br>
+ // to be merged. In COFF, it's not very easy to get the section attribute<br>
+ // for the symbol, so scan all sections in advance and cache the attributes<br>
+ // for later use.<br>
+ if (error_code ec = cacheSectionAttributes())<br>
+ return ec;<br>
+<br>
// Filter non-defined atoms, and group defined atoms by its section.<br>
SectionToSymbolsT definedSymbols;<br>
for (const coff_symbol *sym : symbols) {<br>
@@ -223,6 +242,22 @@ private:<br>
sym->SectionNumber == llvm::COFF::IMAGE_SYM_UNDEFINED)<br>
continue;<br>
<br>
+ const coff_section *sec;<br>
+ if (error_code ec = _obj->getSection(sym->SectionNumber, sec))<br>
+ return ec;<br>
+ assert(sec && "SectionIndex > 0, Sec must be non-null!");<br>
+<br>
+ // SKip if it's a section symbol for a COMDAT section. A section symbol<br></blockquote><div><br></div><div>Typo on SKip.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ // has the name of the section and value 0. A translation unit may contain<br>
+ // multiple COMDAT sections whose section name are the same. We don't want<br>
+ // to make atoms for them as they would become duplicate symbols.<br>
+ StringRef sectionName;<br>
+ if (error_code ec = _obj->getSectionName(sec, sectionName))<br>
+ return ec;<br>
+ if (_symbolName[sym] == sectionName && sym->Value == 0 &&<br>
+ _merge[sec] != DefinedAtom::mergeNo)<br>
+ continue;<br>
+<br>
uint8_t sc = sym->StorageClass;<br>
if (sc != llvm::COFF::IMAGE_SYM_CLASS_EXTERNAL &&<br>
sc != llvm::COFF::IMAGE_SYM_CLASS_STATIC &&<br>
@@ -233,10 +268,6 @@ private:<br>
return llvm::object::object_error::parse_failed;<br>
}<br>
<br>
- const coff_section *sec;<br>
- if (error_code ec = _obj->getSection(sym->SectionNumber, sec))<br>
- return ec;<br>
- assert(sec && "SectionIndex > 0, Sec must be non-null!");<br>
definedSymbols[sec].push_back(sym);<br>
}<br>
<br>
@@ -247,6 +278,55 @@ private:<br>
return error_code::success();<br>
}<br>
<br>
+ // Cache the COMDAT attributes, which indicate whether the symbols in the<br>
+ // section can be merged or not.<br>
+ error_code cacheSectionAttributes() {<br>
+ const llvm::object::coff_file_header *header = nullptr;<br>
+ if (error_code ec = _obj->getHeader(header))<br>
+ return ec;<br>
+<br>
+ // The COMDAT section attribute is not an attribute of coff_section, but is<br>
+ // stored in the auxiliary symbol for the first symbol referring a COMDAT<br>
+ // section. It feels to me that it's unnecessarily complicated, but this is<br>
+ // how COFF works.<br>
+ for (uint32_t i = 0, e = header->NumberOfSymbols; i != e; ++i) {<br>
+ const coff_symbol *sym;<br>
+ if (error_code ec = _obj->getSymbol(i, sym))<br>
+ return ec;<br>
+ if (sym->SectionNumber == llvm::COFF::IMAGE_SYM_ABSOLUTE ||<br>
+ sym->SectionNumber == llvm::COFF::IMAGE_SYM_UNDEFINED)<br>
+ continue;<br>
+<br>
+ const coff_section *sec;<br>
+ if (error_code ec = _obj->getSection(sym->SectionNumber, sec))<br>
+ return ec;<br>
+<br>
+ if (_merge.count(sec))<br>
+ continue;<br>
+ if (!(sec->Characteristics & llvm::COFF::IMAGE_SCN_LNK_COMDAT))<br>
+ continue;<br>
+<br>
+ if (sym->NumberOfAuxSymbols == 0)<br>
+ return llvm::object::object_error::parse_failed;<br>
+ const coff_aux_section_definition *aux = nullptr;<br>
+ if (error_code ec = _obj->getAuxSymbol(i + 1, aux))<br>
+ return ec;<br>
+<br>
+ _merge[sec] = getMerge(aux);<br>
+ }<br>
+<br>
+ // The sections that does not have auxiliary symbol are regular sections, in<br>
+ // which symbols are not allowed to be merged.<br>
+ error_code ec;<br>
+ for (auto si = _obj->begin_sections(), se = _obj->end_sections(); si != se;<br>
+ si.increment(ec)) {<br>
+ const coff_section *sec = _obj->getCOFFSection(si);<br>
+ if (!_merge.count(sec))<br>
+ _merge[sec] = DefinedAtom::mergeNo;<br>
+ }<br>
+ return error_code::success();<br>
+ }<br>
+<br>
/// Atomize \p symbols and append the results to \p atoms. The symbols are<br>
/// assumed to have been defined in the \p section.<br>
error_code<br>
@@ -310,7 +390,7 @@ private:<br>
ArrayRef<uint8_t> data(secData.data(), secData.size());<br>
auto *atom = new (_alloc) COFFDefinedAtom(<br>
*this, "", sectionName, Atom::scopeTranslationUnit, type, perms,<br>
- data, 0);<br>
+ _merge[section], data, 0);<br>
atoms.push_back(atom);<br>
_definedAtomLocations[section][0] = atom;<br>
return error_code::success();<br>
@@ -323,7 +403,7 @@ private:<br>
ArrayRef<uint8_t> data(secData.data(), size);<br>
auto *atom = new (_alloc) COFFDefinedAtom(<br>
*this, "", sectionName, Atom::scopeTranslationUnit, type, perms,<br>
- data, ++ordinal);<br>
+ _merge[section], data, ++ordinal);<br>
atoms.push_back(atom);<br>
_definedAtomLocations[section][0] = atom;<br>
}<br>
@@ -337,7 +417,7 @@ private:<br>
ArrayRef<uint8_t> data(start, end);<br>
auto *atom = new (_alloc) COFFDefinedAtom(<br>
*this, _symbolName[*si], sectionName, getScope(*si), type, perms,<br>
- data, ++ordinal);<br>
+ _merge[section], data, ++ordinal);<br>
atoms.push_back(atom);<br>
_symbolAtom[*si] = atom;<br>
_definedAtomLocations[section][(*si)->Value] = atom;<br>
@@ -519,6 +599,9 @@ private:<br>
// A map from section to its atoms.<br>
std::map<const coff_section *, vector<COFFDefinedFileAtom *>> _sectionAtoms;<br>
<br>
+ // A map to get whether the section allows its contents to be merged or not.<br>
+ std::map<const coff_section *, DefinedAtom::Merge> _merge;<br>
+<br>
// A sorted map to find an atom from a section and an offset within<br>
// the section.<br>
std::map<const coff_section *,<br>
<br>
Added: lld/trunk/test/pecoff/Inputs/comdat.obj.yaml<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/test/pecoff/Inputs/comdat.obj.yaml?rev=188025&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/test/pecoff/Inputs/comdat.obj.yaml?rev=188025&view=auto</a><br>
==============================================================================<br>
--- lld/trunk/test/pecoff/Inputs/comdat.obj.yaml (added)<br>
+++ lld/trunk/test/pecoff/Inputs/comdat.obj.yaml Thu Aug 8 18:31:50 2013<br>
@@ -0,0 +1,43 @@<br>
+---<br>
+header:<br>
+ Machine: IMAGE_FILE_MACHINE_I386<br>
+ Characteristics: [ ]<br>
+sections:<br>
+ - Name: .text<br>
+ Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]<br>
+ Alignment: 16<br>
+ SectionData: 558BEC33C05DC3<br>
+ - Name: .text<br>
+ Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_LNK_COMDAT, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]<br>
+ Alignment: 16<br>
+ SectionData: 558BEC33C05DC3<br>
+symbols:<br>
+ - Name: .text<br>
+ Value: 0<br>
+ SectionNumber: 1<br>
+ SimpleType: IMAGE_SYM_TYPE_NULL<br>
+ ComplexType: IMAGE_SYM_DTYPE_NULL<br>
+ StorageClass: IMAGE_SYM_CLASS_STATIC<br>
+ NumberOfAuxSymbols: 1<br>
+ AuxiliaryData: 0700000000000000C979F796000002000000<br>
+ - Name: .text<br>
+ Value: 0<br>
+ SectionNumber: 2<br>
+ SimpleType: IMAGE_SYM_TYPE_NULL<br>
+ ComplexType: IMAGE_SYM_DTYPE_NULL<br>
+ StorageClass: IMAGE_SYM_CLASS_STATIC<br>
+ NumberOfAuxSymbols: 1<br>
+ AuxiliaryData: 0700000000000000C979F796000002000000<br>
+ - Name: "?inlinefn1@@YAHXZ"<br>
+ Value: 0<br>
+ SectionNumber: 1<br>
+ SimpleType: IMAGE_SYM_TYPE_NULL<br>
+ ComplexType: IMAGE_SYM_DTYPE_FUNCTION<br>
+ StorageClass: IMAGE_SYM_CLASS_EXTERNAL<br>
+ - Name: "?inlinefn2@@YAHXZ"<br>
+ Value: 0<br>
+ SectionNumber: 2<br>
+ SimpleType: IMAGE_SYM_TYPE_NULL<br>
+ ComplexType: IMAGE_SYM_DTYPE_FUNCTION<br>
+ StorageClass: IMAGE_SYM_CLASS_EXTERNAL<br>
+...<br>
<br>
Added: lld/trunk/test/pecoff/comdat.test<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/test/pecoff/comdat.test?rev=188025&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/test/pecoff/comdat.test?rev=188025&view=auto</a><br>
==============================================================================<br>
--- lld/trunk/test/pecoff/comdat.test (added)<br>
+++ lld/trunk/test/pecoff/comdat.test Thu Aug 8 18:31:50 2013<br>
@@ -0,0 +1,12 @@<br>
+# RUN: yaml2obj %p/Inputs/comdat.obj.yaml > %t1.obj<br>
+# RUN: yaml2obj %p/Inputs/comdat.obj.yaml > %t2.obj<br>
+#<br>
+# RUN: lld -flavor link /out:%t3.exe /subsystem:console /force \<br>
+# RUN: -- %t1.obj %t2.obj 2>&1 > %t.log<br>
+#<br>
+# FileCheck complains if the input files is empty, so add a dummy line.<br>
+# RUN: echo foo >> %t.log<br>
+#<br>
+# RUN: FileCheck %s < %t.log<br>
+<br>
+CHECK-NOT: duplicate symbol error<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>