<div dir="ltr">On Thu, Aug 8, 2013 at 4:46 PM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@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">
<div dir="ltr"><div><div class="h5">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></div><div class="gmail_extra">
<div class="gmail_quote"><div><div class="h5"><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></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></div></div></blockquote><div><br></div><div>Fully cover the switch seems a good idea but mapping to mergeNo is not, because the attributes are similar to the weak symbol rather than the regular one. If we would map it to mergeNo, we can't link a simple program until we fully support these semantics in the core linker.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">
<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></div><div>Typo on SKip.</div><div><div class="h5"><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" target="_blank">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></div></div><br></div></div>
</blockquote></div><br></div></div>