<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="font-size:12.8px">> Index: lib/Object/ArchiveWriter.cpp<br></span><span style="font-size:12.8px">> ==============================</span><wbr style="font-size:12.8px"><span style="font-size:12.8px">==============================</span><wbr style="font-size:12.8px"><span style="font-size:12.8px">=======<br></span><span style="font-size:12.8px">> --- lib/Object/ArchiveWriter.cpp<br></span><span style="font-size:12.8px">> +++ lib/Object/ArchiveWriter.cpp<br></span><span style="font-size:12.8px">> @@ -314,7 +314,8 @@<br></span><span style="font-size:12.8px">> continue;<br></span><span style="font-size:12.8px">> if (!(Symflags & object::SymbolRef::SF_Global))<br></span><span style="font-size:12.8px">> continue;<br></span><span style="font-size:12.8px">> - if (Symflags & object::SymbolRef::SF_</span><wbr style="font-size:12.8px"><span style="font-size:12.8px">Undefined)<br></span><span class="gmail-im" style="font-size:12.8px">> + if (Symflags & object::SymbolRef::SF_<wbr>Undefined &&<br></span><span class="gmail-im" style="font-size:12.8px">> + !(Symflags & object::SymbolRef::SF_Weak))<br></span><span class="gmail-im" style="font-size:12.8px">> continue;</span><span class="gmail-im" style="font-size:12.8px"><br></span><span style="font-size:12.8px">This does look wrong for ELF. In any case, I will just check in a<br></span><span style="font-size:12.8px">testcase for ELF that will show the issue.</span></blockquote><div>Great, Thanks Rafael.<br>This should help a lot to ensure we don't break any existing functionality.<br><br>Suggestions are welcome on how best to make this happen for COFF only.<br>Passing magic boolean flags might not be the correct way.<br><br>Best,<br>Martell</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 15, 2017 at 4:22 PM, Rafael Avila de Espindola <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"><div class="HOEnZb"><div class="h5">Martell Malone via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> writes:<br>
<br>
> martell updated this revision to Diff 98637.<br>
> martell added a comment.<br>
> Herald added a subscriber: inglorion.<br>
><br>
> light update to take <a href="https://reviews.llvm.org/D32689" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D32689</a> into account.<br>
><br>
> note:<br>
> also seems `llvm-readobj` can't read weak externals<br>
> will have to add support on top of the short import libs support I done previously.<br>
><br>
><br>
> Repository:<br>
> rL LLVM<br>
><br>
> <a href="https://reviews.llvm.org/D29892" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D29892</a><br>
><br>
> Files:<br>
> include/llvm/DlltoolDriver/<wbr>DlltoolDriver.h<br>
> include/llvm/Object/<wbr>COFFImportFile.h<br>
> include/llvm/Object/<wbr>COFFModuleDefinition.h<br>
> lib/CMakeLists.txt<br>
> lib/DlltoolDriver/CMakeLists.<wbr>txt<br>
> lib/DlltoolDriver/<wbr>DlltoolDriver.cpp<br>
> lib/DlltoolDriver/LLVMBuild.<wbr>txt<br>
> lib/DlltoolDriver/Options.td<br>
> lib/Object/ArchiveWriter.cpp<br>
> lib/Object/COFFImportFile.cpp<br>
> lib/Object/<wbr>COFFModuleDefinition.cpp<br>
> test/DllTool/coff-exports.def<br>
> test/DllTool/lit.local.cfg<br>
> tools/llvm-ar/CMakeLists.txt<br>
> tools/llvm-ar/llvm-ar.cpp<br>
><br>
</div></div>> Index: tools/llvm-ar/llvm-ar.cpp<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- tools/llvm-ar/llvm-ar.cpp<br>
> +++ tools/llvm-ar/llvm-ar.cpp<br>
> @@ -16,6 +16,7 @@<br>
> #include "llvm/ADT/Triple.h"<br>
> #include "llvm/IR/LLVMContext.h"<br>
> #include "llvm/IR/Module.h"<br>
> +#include "llvm/DlltoolDriver/<wbr>DlltoolDriver.h"<br>
> #include "llvm/LibDriver/LibDriver.h"<br>
> #include "llvm/Object/Archive.h"<br>
> #include "llvm/Object/ArchiveWriter.h"<br>
> @@ -859,6 +860,9 @@<br>
> llvm::InitializeAllAsmParsers(<wbr>);<br>
><br>
> StringRef Stem = sys::path::stem(ToolName);<br>
> + if (Stem.find("dlltool") != StringRef::npos)<br>
> + return dlltoolDriverMain(<wbr>makeArrayRef(argv, argc));<br>
> +<br>
> if (Stem.find("ranlib") == StringRef::npos &&<br>
> Stem.find("lib") != StringRef::npos)<br>
> return libDriverMain(makeArrayRef(<wbr>argv, argc));<br>
> @@ -874,5 +878,5 @@<br>
> return ranlib_main();<br>
> if (Stem.find("ar") != StringRef::npos)<br>
> return ar_main();<br>
> - fail("Not ranlib, ar or lib!");<br>
> + fail("Not ranlib, ar, lib or dlltool!");<br>
> }<br>
> Index: tools/llvm-ar/CMakeLists.txt<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- tools/llvm-ar/CMakeLists.txt<br>
> +++ tools/llvm-ar/CMakeLists.txt<br>
> @@ -1,6 +1,7 @@<br>
> set(LLVM_LINK_COMPONENTS<br>
> ${LLVM_TARGETS_TO_BUILD}<br>
> Core<br>
> + DlltoolDriver<br>
> LibDriver<br>
> Object<br>
> Support<br>
> @@ -15,3 +16,4 @@<br>
><br>
> add_llvm_tool_symlink(llvm-<wbr>ranlib llvm-ar)<br>
> add_llvm_tool_symlink(llvm-lib llvm-ar)<br>
> +add_llvm_tool_symlink(llvm-<wbr>dlltool llvm-ar)<br>
> Index: test/DllTool/lit.local.cfg<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- /dev/null<br>
> +++ test/DllTool/lit.local.cfg<br>
> @@ -0,0 +1 @@<br>
> +config.suffixes = ['.def']<br>
> Index: test/DllTool/coff-exports.def<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- /dev/null<br>
> +++ test/DllTool/coff-exports.def<br>
> @@ -0,0 +1,13 @@<br>
> +; RUN: llvm-dlltool -a x86_64 %s %t.a<br>
> +; RUN: llvm-readobj -coff-exports %t.a | FileCheck %s<br>
> +<br>
> +LIBRARY test.dll<br>
> +EXPORTS<br>
> +TestFunction<br>
> +<br>
> +; CHECK: File: test.dll<br>
> +; CHECK: Format: COFF-import-file<br>
> +; CHECK: Type: code<br>
> +; CHECK: Name type: name<br>
> +; CHECK: Symbol: __imp_TestFunction<br>
> +; CHECK: Symbol: TestFunction<br>
> Index: lib/Object/<wbr>COFFModuleDefinition.cpp<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- lib/Object/<wbr>COFFModuleDefinition.cpp<br>
> +++ lib/Object/<wbr>COFFModuleDefinition.cpp<br>
> @@ -55,8 +55,10 @@<br>
> StringRef Value;<br>
> };<br>
><br>
> -static bool isDecorated(StringRef Sym) {<br>
> - return Sym.startswith("_") || Sym.startswith("@") || Sym.startswith("?");<br>
> +static bool isDecorated(StringRef Sym, bool MingwDef) {<br>
> + // Disable Sym.startswith("_") for mingw-w64<br>
> + return (!MingwDef && Sym.startswith("_")) ||<br>
> + Sym.startswith("@") || Sym.startswith("?");<br>
> }<br>
><br>
> static Error createError(const Twine &Err) {<br>
> @@ -82,6 +84,10 @@<br>
> return lex();<br>
> }<br>
> case '=':<br>
> + // This lets us handle "==" which is a dlltool ism<br>
> + if (Buf[1] == '=')<br>
> + Buf = Buf.drop_front();<br>
> +<br>
> Buf = Buf.drop_front();<br>
> return Token(Equal, "=");<br>
> case ',':<br>
> @@ -120,7 +126,8 @@<br>
><br>
> class Parser {<br>
> public:<br>
> - explicit Parser(StringRef S, MachineTypes M) : Lex(S), Machine(M) {}<br>
> + explicit Parser(StringRef S, MachineTypes M,<br>
> + bool B) : Lex(S), Machine(M), MingwDef(B) {}<br>
><br>
> Expected<COFFModuleDefinition> parse() {<br>
> do {<br>
> @@ -213,9 +220,9 @@<br>
> }<br>
><br>
> if (Machine == IMAGE_FILE_MACHINE_I386) {<br>
> - if (!isDecorated(E.Name))<br>
> + if (!isDecorated(E.Name, MingwDef))<br>
> E.Name = (std::string("_").append(E.<wbr>Name));<br>
> - if (!E.ExtName.empty() && !isDecorated(E.ExtName))<br>
> + if (!E.ExtName.empty() && !isDecorated(E.ExtName, MingwDef))<br>
> E.ExtName = (std::string("_").append(E.<wbr>ExtName));<br>
> }<br>
><br>
> @@ -244,6 +251,12 @@<br>
> continue;<br>
> }<br>
> unget();<br>
> +<br>
> + if (E.isWeak())<br>
> + E.SymbolName = E.ExtName;<br>
> + else<br>
> + E.SymbolName = E.Name;<br>
> +<br>
> Info.Exports.push_back(E);<br>
> return Error::success();<br>
> }<br>
> @@ -308,11 +321,13 @@<br>
> std::vector<Token> Stack;<br>
> MachineTypes Machine;<br>
> COFFModuleDefinition Info;<br>
> + bool MingwDef;<br>
> };<br>
><br>
> Expected<COFFModuleDefinition> parseCOFFModuleDefinition(<wbr>MemoryBufferRef MB,<br>
> - MachineTypes Machine) {<br>
> - return Parser(MB.getBuffer(), Machine).parse();<br>
> + MachineTypes Machine,<br>
> + bool MingwDef) {<br>
> + return Parser(MB.getBuffer(), Machine, MingwDef).parse();<br>
> }<br>
><br>
> } // namespace object<br>
> Index: lib/Object/COFFImportFile.cpp<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- lib/Object/COFFImportFile.cpp<br>
> +++ lib/Object/COFFImportFile.cpp<br>
> @@ -162,6 +162,10 @@<br>
> // Library Format.<br>
> NewArchiveMember createShortImport(StringRef Sym, uint16_t Ordinal,<br>
> ImportType Type, ImportNameType NameType);<br>
> +<br>
> + // Create a weak external file which is described in PE/COFF Aux Format 3.<br>
> + NewArchiveMember createWeakExternal(std::<wbr>vector<uint8_t> &Buffer,<br>
> + StringRef Sym, StringRef Weak, bool imp);<br>
> };<br>
> } // namespace<br>
><br>
> @@ -476,6 +480,89 @@<br>
> return {MemoryBufferRef(StringRef(<wbr>Buf, Size), DLLName)};<br>
> }<br>
><br>
> +NewArchiveMember ObjectFactory::<wbr>createWeakExternal(std::<wbr>vector<uint8_t> &Buffer,<br>
> + StringRef Sym, StringRef Weak, bool imp) {<br>
> + static const uint32_t NumberOfSections = 1;<br>
> + static const uint32_t NumberOfSymbols = 5;<br>
> +<br>
> + // COFF Header<br>
> + coff_file_header Header{<br>
> + u16(0), u16(NumberOfSections), u32(0),<br>
> + u32(sizeof(Header) + (NumberOfSections * sizeof(coff_section))),<br>
> + u32(NumberOfSymbols), u16(0),<br>
> + u16(0),<br>
> + };<br>
> + append(Buffer, Header);<br>
> +<br>
> + // Section Header Table<br>
> + static const coff_section SectionTable[NumberOfSections] = {<br>
> + {{'.', 'd', 'r', 'e', 'c', 't', 'v', 'e'},<br>
> + u32(0),<br>
> + u32(0),<br>
> + u32(0),<br>
> + u32(0),<br>
> + u32(0),<br>
> + u32(0),<br>
> + u16(0),<br>
> + u16(0),<br>
> + u32(IMAGE_SCN_LNK_INFO | IMAGE_SCN_LNK_REMOVE)}<br>
> + };<br>
> + append(Buffer, SectionTable);<br>
> +<br>
> + // Symbol Table<br>
> + coff_symbol16 SymbolTable[NumberOfSymbols] = {<br>
> + {{{'@', 'c', 'o', 'm', 'p', '.', 'i', 'd'}},<br>
> + u32(0),<br>
> + u16(0xFFFF),<br>
> + u16(0),<br>
> + IMAGE_SYM_CLASS_STATIC,<br>
> + 0},<br>
> + {{{'@', 'f', 'e', 'a', 't', '.', '0', '0'}},<br>
> + u32(0),<br>
> + u16(0xFFFF),<br>
> + u16(0),<br>
> + IMAGE_SYM_CLASS_STATIC,<br>
> + 0},<br>
> + {{{0, 0, 0, 0, 0, 0, 0, 0}},<br>
> + u32(0),<br>
> + u16(0),<br>
> + u16(0),<br>
> + IMAGE_SYM_CLASS_EXTERNAL,<br>
> + 0},<br>
> + {{{0, 0, 0, 0, 0, 0, 0, 0}},<br>
> + u32(0),<br>
> + u16(0),<br>
> + u16(0),<br>
> + IMAGE_SYM_CLASS_WEAK_EXTERNAL,<br>
> + 1},<br>
> + {{{2, 0, 0, 0, 3, 0, 0, 0}},<br>
> + u32(0),<br>
> + u16(0),<br>
> + u16(0),<br>
> + uint8_t(0),<br>
> + 0},<br>
> + };<br>
> + reinterpret_cast<<wbr>StringTableOffset &>(SymbolTable[2].Name).Offset =<br>
> + sizeof(uint32_t);<br>
> +<br>
> + //__imp_ String Table<br>
> + if(imp) {<br>
> + reinterpret_cast<<wbr>StringTableOffset &>(SymbolTable[3].Name).Offset =<br>
> + sizeof(uint32_t) + Sym.size() + 1 + 6;<br>
> + writeStringTable(Buffer, {std::string("__imp_").append(<wbr>Sym),<br>
> + std::string("__imp_").append(<wbr>Weak)});<br>
> + } else {<br>
> + reinterpret_cast<<wbr>StringTableOffset &>(SymbolTable[3].Name).Offset =<br>
> + sizeof(uint32_t) + Sym.size() + 1;<br>
> + writeStringTable(Buffer, {Sym, Weak});<br>
> + }<br>
> + append(Buffer, SymbolTable);<br>
> +<br>
> + StringRef F{reinterpret_cast<const char *>(Buffer.data()), Buffer.size()};<br>
> + return {MemoryBufferRef{F, DLLName}};<br>
> +<br>
> +}<br>
> +<br>
> std::error_code writeImportLibrary(StringRef DLLName, StringRef Path,<br>
> ArrayRef<COFFShortExport> Exports,<br>
> MachineTypes Machine) {<br>
> @@ -496,6 +583,14 @@<br>
> if (E.Private)<br>
> continue;<br>
><br>
> + if (E.isWeak()) {<br>
> + std::vector<uint8_t> *WeakBuffer1 = new std::vector<uint8_t>();<br>
> + std::vector<uint8_t> *WeakBuffer2 = new std::vector<uint8_t>();<br>
> + Members.push_back(OF.<wbr>createWeakExternal(*<wbr>WeakBuffer1, E.Name, E.ExtName, false));<br>
> + Members.push_back(OF.<wbr>createWeakExternal(*<wbr>WeakBuffer2, E.Name, E.ExtName, true));<br>
> + continue;<br>
> + }<br>
> +<br>
> ImportType ImportType = IMPORT_CODE;<br>
> if (E.Data)<br>
> ImportType = IMPORT_DATA;<br>
> Index: lib/Object/ArchiveWriter.cpp<br>
> ==============================<wbr>==============================<wbr>=======<br>
> --- lib/Object/ArchiveWriter.cpp<br>
> +++ lib/Object/ArchiveWriter.cpp<br>
> @@ -314,7 +314,8 @@<br>
> continue;<br>
> if (!(Symflags & object::SymbolRef::SF_Global))<br>
> continue;<br>
> - if (Symflags & object::SymbolRef::SF_<wbr>Undefined)<br>
<span class="">> + if (Symflags & object::SymbolRef::SF_<wbr>Undefined &&<br>
> + !(Symflags & object::SymbolRef::SF_Weak))<br>
> continue;<br>
<br>
</span>This does look wrong for ELF. In any case, I will just check in a<br>
testcase for ELF that will show the issue.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div>