<div dir="ltr">We could, but now LLD cannot produce duplicate entries, so we cannot make it emit two versions and compare them. I could write a test that verifies that file size of output is less than some value, but file size could change for other reasons, I'm reluctant to write such test.<div><br></div><div>Maybe we could write a test to check if some string is contained only once in a file?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 24, 2015 at 4:39 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">well, you could test that the binary is now smaller, no?<br>
<div class="HOEnZb"><div class="h5"><br>
On 24 March 2015 at 19:23, Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> wrote:<br>
> The existing test cases using llvm-readobj should suffice. Output from<br>
> readobj didn't change, as the tool doesn't care whether or not the string<br>
> it's printing is an alias of something.<br>
><br>
> On Tue, Mar 24, 2015 at 4:21 PM, Rafael Espíndola<br>
> <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
>><br>
>> testcase? :-)<br>
>><br>
>> On 24 March 2015 at 18:43, Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> wrote:<br>
>> > Author: ruiu<br>
>> > Date: Tue Mar 24 17:43:58 2015<br>
>> > New Revision: 233128<br>
>> ><br>
>> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=233128&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=233128&view=rev</a><br>
>> > Log:<br>
>> > PECOFF: Reduce import table size.<br>
>> ><br>
>> > Import Lookup Table in Import Directory Table has the same contents<br>
>> > as Hint/Name Table. Symbol names imported from DLLs are pointed by<br>
>> > both Import Directory Table and Hint/Name Table. We had duplicate<br>
>> > strings there.<br>
>> ><br>
>> > This patch eliminates that duplication to make the table smaller.<br>
>> > This should reduce binary size by the sum of lengths of imported<br>
>> > symbols.<br>
>> ><br>
>> > Modified:<br>
>> > lld/trunk/lib/ReaderWriter/PECOFF/IdataPass.cpp<br>
>> ><br>
>> > Modified: lld/trunk/lib/ReaderWriter/PECOFF/IdataPass.cpp<br>
>> > URL:<br>
>> > <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/PECOFF/IdataPass.cpp?rev=233128&r1=233127&r2=233128&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/lib/ReaderWriter/PECOFF/IdataPass.cpp?rev=233128&r1=233127&r2=233128&view=diff</a><br>
>> ><br>
>> > ==============================================================================<br>
>> > --- lld/trunk/lib/ReaderWriter/PECOFF/IdataPass.cpp (original)<br>
>> > +++ lld/trunk/lib/ReaderWriter/PECOFF/IdataPass.cpp Tue Mar 24 17:43:58<br>
>> > 2015<br>
>> > @@ -69,6 +69,7 @@ static std::vector<ImportTableEntryAtom<br>
>> > createImportTableAtoms(IdataContext &context,<br>
>> > const std::vector<COFFSharedLibraryAtom *><br>
>> > &sharedAtoms,<br>
>> > bool shouldAddReference, StringRef sectionName,<br>
>> > + std::map<StringRef, HintNameAtom *><br>
>> > &hintNameCache,<br>
>> > llvm::BumpPtrAllocator &alloc) {<br>
>> > std::vector<ImportTableEntryAtom *> ret;<br>
>> > for (COFFSharedLibraryAtom *atom : sharedAtoms) {<br>
>> > @@ -81,9 +82,16 @@ createImportTableAtoms(IdataContext &con<br>
>> > } else {<br>
>> > // Import by name<br>
>> > entry = new (alloc) ImportTableEntryAtom(context, 0,<br>
>> > sectionName);<br>
>> > - HintNameAtom *hintName =<br>
>> > - new (alloc) HintNameAtom(context, atom->hint(),<br>
>> > atom->importName());<br>
>> > - addDir32NBReloc(entry, hintName, context.ctx.getMachineType(),<br>
>> > 0);<br>
>> > + HintNameAtom *hintNameAtom;<br>
>> > + auto it = hintNameCache.find(atom->importName());<br>
>> > + if (it == hintNameCache.end()) {<br>
>> > + hintNameAtom = new (alloc) HintNameAtom(<br>
>> > + context, atom->hint(), atom->importName());<br>
>> > + hintNameCache[atom->importName()] = hintNameAtom;<br>
>> > + } else {<br>
>> > + hintNameAtom = it->second;<br>
>> > + }<br>
>> > + addDir32NBReloc(entry, hintNameAtom,<br>
>> > context.ctx.getMachineType(), 0);<br>
>> > }<br>
>> > ret.push_back(entry);<br>
>> > if (shouldAddReference)<br>
>> > @@ -104,10 +112,13 @@ void ImportDirectoryAtom::addRelocations<br>
>> > // same. The PE/COFF loader overwrites the import address tables with<br>
>> > the<br>
>> > // pointers to the referenced items after loading the executable into<br>
>> > // memory.<br>
>> > + std::map<StringRef, HintNameAtom *> hintNameCache;<br>
>> > std::vector<ImportTableEntryAtom *> importLookupTables =<br>
>> > - createImportTableAtoms(context, sharedAtoms, false, ".idata.t",<br>
>> > _alloc);<br>
>> > + createImportTableAtoms(context, sharedAtoms, false, ".idata.t",<br>
>> > + hintNameCache, _alloc);<br>
>> > std::vector<ImportTableEntryAtom *> importAddressTables =<br>
>> > - createImportTableAtoms(context, sharedAtoms, true, ".idata.a",<br>
>> > _alloc);<br>
>> > + createImportTableAtoms(context, sharedAtoms, true, ".idata.a",<br>
>> > + hintNameCache, _alloc);<br>
>> ><br>
>> > addDir32NBReloc(this, importLookupTables[0],<br>
>> > context.ctx.getMachineType(),<br>
>> > offsetof(ImportDirectoryTableEntry,<br>
>> > ImportLookupTableRVA));<br>
>> > @@ -157,8 +168,10 @@ void DelayImportDirectoryAtom::addReloca<br>
>> > // as (non-delay) import table's Import Lookup Table. Contains<br>
>> > // imported function names. This is a parallel array of AddressTable<br>
>> > // field.<br>
>> > + std::map<StringRef, HintNameAtom *> hintNameCache;<br>
>> > std::vector<ImportTableEntryAtom *> nameTable =<br>
>> > - createImportTableAtoms(context, sharedAtoms, false, ".didat",<br>
>> > _alloc);<br>
>> > + createImportTableAtoms(<br>
>> > + context, sharedAtoms, false, ".didat", hintNameCache,<br>
>> > _alloc);<br>
>> > addDir32NBReloc(<br>
>> > this, nameTable[0], context.ctx.getMachineType(),<br>
>> > offsetof(delay_import_directory_table_entry,<br>
>> > DelayImportNameTable));<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>
><br>
><br>
</div></div></blockquote></div><br></div>