[PATCH] Change representation of dllexport/dllimport

[Bill Wendling]

On Jul 7, 2013, at 12:35 PM, Reid Kleckner <rnk at google.com> wrote:

> Thanks for working on this!
> 
> Some links to previous discussion for anyone else following along.
> The original design RFC on llvmdev: http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-March/060646.html
> The combined clang/llvm patch with a more detailed CL description: http://llvm-reviews.chandlerc.com/D1099
> 
> Phab is down, so I'll just make some high level comments.
> 
> 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.
> 
We all should have a vested interest in the LLVM IL. :-)

My initial thought would be that adding new linkage types would be the way to go. However, if there are going to be a lot of them, then it's a combinatoric nightmare. If you can mix all other linkage types with the dll* types, then maybe it should be a new field (sub-linkage type?) in the GlobalValue object. Or even expand the size of the linkage field and make it a bit-mask with the dll linkage types.

As for making it an attribute, I'm not against this, but it does seem like a departure from how other linkage types are specified.

I don't know what's best here. But I have a slight preference for the non-attributes way of doing things, because it seems more consistent. So you'll have something like this:

	declare dllexport linkonce_odr void @foo()

Internally, it would be represented like this:

class GlobalValue : public Constant {
//...
public:
  enum LinkageTypes {
//...
    LinkeOnceODRLinkage,
//...
    DLLImportLinkage = 0x10000,
    DLLExprtLinkage  = 0x20000
  };

  unsigned Linkage : 19;
//...
};

With equivalent modifications for querying the linkage type.

-bw

> 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.
> 
> 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 http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007b_005f_005fdeclspec_0028dllexport_0029_007d-2630, meaning you can't combine them with gcc.
> 
> At this point, I've convinced myself that the export control stuff is best modeled as a new visibility.
> 
> 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.
> 
> 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.
> 
> 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.
> 
> 
> On Fri, Jul 5, 2013 at 9:38 PM, Nico Rieck <nico.rieck at gmail.com> wrote:
> Representing dllexport/dllimport as distinct linkage types prevents using these attributes on templates and inline functions.
> 
> 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:
> 
>       @Exp = global i32 1, align 4, dllexport
>       @Imp = external global i32, dllimport
> 
> 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.
> 
> http://llvm-reviews.chandlerc.com/D1110
> 
> Files:
>   docs/LangRef.rst
>   include/llvm-c/Core.h
>   include/llvm/IR/Attributes.h
>   include/llvm/IR/GlobalValue.h
>   include/llvm/IR/GlobalVariable.h
>   lib/AsmParser/LLParser.cpp
>   lib/Bitcode/Reader/BitcodeReader.cpp
>   lib/Bitcode/Writer/BitcodeWriter.cpp
>   lib/CodeGen/AsmPrinter/AsmPrinter.cpp
>   lib/ExecutionEngine/ExecutionEngine.cpp
>   lib/IR/AsmWriter.cpp
>   lib/IR/Attributes.cpp
>   lib/IR/Core.cpp
>   lib/IR/Globals.cpp
>   lib/IR/Verifier.cpp
>   lib/Linker/LinkModules.cpp
>   lib/Target/CppBackend/CPPBackend.cpp
>   lib/Target/Mangler.cpp
>   lib/Target/X86/X86AsmPrinter.cpp
>   lib/Target/X86/X86FastISel.cpp
>   lib/Target/X86/X86ISelLowering.cpp
>   lib/Target/X86/X86Subtarget.cpp
>   lib/Target/XCore/XCoreAsmPrinter.cpp
>   test/CodeGen/X86/dll-linkage.ll
>   test/CodeGen/X86/dllexport-x86_64.ll
>   test/CodeGen/X86/dllexport.ll
>   test/CodeGen/X86/dllimport-x86_64.ll
>   test/CodeGen/X86/dllimport.ll
>   test/MC/COFF/linker-options.ll
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits