<div dir="ltr"><div style>Thanks for working on this!</div><div><br></div><div>Some links to previous discussion for anyone else following along.</div><div>The original design RFC on llvmdev: <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-March/060646.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-March/060646.html</a></div>
<div>The combined clang/llvm patch with a more detailed CL description: <a href="http://llvm-reviews.chandlerc.com/D1099" target="_blank">http://llvm-reviews.chandlerc.com/D1099</a></div><div><br></div><div>Phab is down, so I'll just make some high level comments.</div>
<div><br></div><div>From the original 4 approaches, it looks like you went with #4 (new attributes). Adding new visibilities (#3) is still on the table if people don't like the duplication between global variables and functions. I'd like to hear from someone with more of a vested interest in the LLVM IL.</div>
<div><br></div><div style>In the original RFC, you suggested that someone might want to mix visibility and dll export control. Is that a real use case? I don't think PE DLLs can actually implement ELF's simple, C-like global namespace, so it sounds like you want to be able to emit ELF files using dllexport.</div>
<div style><br></div><div style>In that case, I don't think there are any valid combinations of visibility + dllexport. dllexport is closest to visibility(default), plus weak_odr instead of linkonce_odr for inline functions. dllimport, as previously mentioned, is closer to available_externally with some tweaks to use *__imp_foo. Finally, GCC's docs say that dllexport implies visibility default <a href="http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007b_005f_005fdeclspec_0028dllexport_0029_007d-2630">http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007b_005f_005fdeclspec_0028dllexport_0029_007d-2630</a>, meaning you can't combine them with gcc.</div>
<div><br></div><div style>At this point, I've convinced myself that the export control stuff is best modeled as a new visibility.</div><div><br></div><div>Regardless of attributes vs. visibilities, you shouldn't break the C API. The C++ API is always in flux, but the C API is stable. You can't remove the linkage enums from llvm-c/Core.h, or at least you should keep the subsequent values stable. See LLVMGhostLinkage for prior art.<br>
</div><div><br></div><div style>You should also auto-upgrade decoded bitcode. Make GetDecodedLinkage() return ExternalLinkage for the old codes for both dllexport and dllimport linkages, and then add another helper to apply the correct attribute or visibility.</div>
<div>
<div><br></div></div><div>If you do end up changing the grammar for global variables, you should add a "Syntax:" block to LangRef for globals like there is for most other language constructs.</div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">On Fri, Jul 5, 2013 at 9:38 PM, Nico Rieck <span dir="ltr"><<a href="mailto:nico.rieck@gmail.com" target="_blank">nico.rieck@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Representing dllexport/dllimport as distinct linkage types prevents using these attributes on templates and inline functions.<br>
<br>
Instead of introducing further mixed linkage types to include linkonce and weak ODR, the old import/export linkage types are replaced with new function attributes (DLLExport, DLLImport) and an extension to global variables:<br>
<br>
@Exp = global i32 1, align 4, dllexport<br>
@Imp = external global i32, dllimport<br>
<br>
Linkage for dllexported globals and functions is now equal to their linkage without dllexport. Imported globals and functions must be either declarations, or definitions with AvailableExternallyLinkage. An alias is exported if the aliasee is exported.<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D1110" target="_blank">http://llvm-reviews.chandlerc.com/D1110</a><br>
<br>
Files:<br>
docs/LangRef.rst<br>
include/llvm-c/Core.h<br>
include/llvm/IR/Attributes.h<br>
include/llvm/IR/GlobalValue.h<br>
include/llvm/IR/GlobalVariable.h<br>
lib/AsmParser/LLParser.cpp<br>
lib/Bitcode/Reader/BitcodeReader.cpp<br>
lib/Bitcode/Writer/BitcodeWriter.cpp<br>
lib/CodeGen/AsmPrinter/AsmPrinter.cpp<br>
lib/ExecutionEngine/ExecutionEngine.cpp<br>
lib/IR/AsmWriter.cpp<br>
lib/IR/Attributes.cpp<br>
lib/IR/Core.cpp<br>
lib/IR/Globals.cpp<br>
lib/IR/Verifier.cpp<br>
lib/Linker/LinkModules.cpp<br>
lib/Target/CppBackend/CPPBackend.cpp<br>
lib/Target/Mangler.cpp<br>
lib/Target/X86/X86AsmPrinter.cpp<br>
lib/Target/X86/X86FastISel.cpp<br>
lib/Target/X86/X86ISelLowering.cpp<br>
lib/Target/X86/X86Subtarget.cpp<br>
lib/Target/XCore/XCoreAsmPrinter.cpp<br>
test/CodeGen/X86/dll-linkage.ll<br>
test/CodeGen/X86/dllexport-x86_64.ll<br>
test/CodeGen/X86/dllexport.ll<br>
test/CodeGen/X86/dllimport-x86_64.ll<br>
test/CodeGen/X86/dllimport.ll<br>
test/MC/COFF/linker-options.ll<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></blockquote></div><br></div>