This is an archive of the discontinued LLVM Phabricator instance.

Change representation of dllexport/dllimport
ClosedPublic

Authored by nrieck on Jul 5 2013, 6:38 PM.

Details

Reviewers
rnk
asl
Summary

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.

Diff Detail

Event Timeline

nrieck updated this revision to Unknown Object (????).Aug 13 2013, 8:11 AM
  • Replaced the mixed representation (function attribute + global var extension) with a new visibility-like specifier, usable for functions, globals and aliases. Note: Aliases could have dllexport linkage before.
  • Kept the old linkage types in the C API, and marked them as obsolete.
  • Extended bitcode for the new specifier, upgrading older bitcode files.
rnk added a comment.Aug 14 2013, 10:07 AM

I still feel strongly that dllexport and dllimport should be new visibility values.

Is there any compelling reason to support a combination of dllimport/dllexport + visibility(hidden/internal/protected)? If not, I don't see any reason to make that representable in IR.

test/CodeGen/X86/dllimport-x86_64.ll
28

Should this be FAST-NEXT: or have some other way to avoid matching one of the calls from below?

test/CodeGen/X86/dllimport.ll
32

ditto

Oh, you meant adding them to GlobalValue::VisibilityTypes? I don't really like referring to dllexport/dllimport as visibilities, especially not dllimport. Though in the end I care more about getting this in. I agree that there's a certain duplication in the IR reader/writer. Regarding combining them, I wonder what the ARM compiler does. Browsing through their reference, they seem to support dllexport/dllimport using ELF (and also visibility). I can't find anything in the docs about issues combining them though. While dllexport must always have default visibility, dllimport could be any.

test/CodeGen/X86/dllimport-x86_64.ll
28

Yeah, would be better.

rnk added a comment.Sep 9 2013, 9:36 AM

How's this patch doing? It's come up on the mailing lists a few times, so I thought I'd ping. :)

Why do you say that dllimport could be any visibility? Is it useful to have a visibility(non-hidden) + dllimport symbol?

asl added a comment.Sep 9 2013, 11:07 AM
Why do you say that dllimport could be any visibility?  Is it useful to have a visibility(non-hidden) + dllimport symbol?

Because dllimport has nothing wrt the visibility. Visibility is
essnetially ELF'ish thing. dllexport can be thought as "default"
visibility and everything else is hidden visibility by default, but
dllimport does not fit into this semantics. Trying to fit them into
visibility is essentialy a workaround

compnerd added inline comments.Sep 10 2013, 10:37 AM
lib/Bitcode/Reader/BitcodeReader.cpp
193

missing return or break

194

ditto

nrieck updated this revision to Unknown Object (????).Nov 26 2013, 10:16 AM
  • Rebased to recent ToT.
  • Fixed UpgradeDLLImportExportLinkage.
  • Added mingw triple to tests.
asl added a reviewer: asl.Nov 27 2013, 2:08 AM
nrieck updated this revision to Unknown Object (????).Jan 8 2014, 8:08 AM
  • Rebased to ToT
rnk accepted this revision.Jan 13 2014, 5:09 PM

Thanks, LGTM!

Sorry for taking a while to get back to this, it's large.

nrieck closed this revision.Jan 14 2014, 10:30 AM

Committed in r199203, r199218 and r199219. Clang changes in r199220.

lib/Target/XCore/XCoreAsmPrinter.cpp