This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Remove dllimport attribute from locally defined symbols
ClosedPublic

Authored by ormris on Dec 12 2018, 4:13 PM.

Details

Summary

The LTO/ThinLTO driver currently creates invalid bitcode by setting
symbols marked dllimport as dso_local. The compiler often has access
to the definition (often dllexport) and the declaration (often
dllimport) of an object at link-time, leading to a conflicting
declaration. This patch resolves the inconsistency by removing the
dllimport attribute.

Diff Detail

Event Timeline

ormris created this revision.Dec 12 2018, 4:13 PM
rnk added a comment.Dec 13 2018, 8:49 AM

Why add dllexport instead of just removing dllimport as was done before? This often happens in situations involving inline methods of templates, and I don't think it would be correct to export such inline functions. I also wouldn't want LTO to suddenly export a bunch of things that the native linker wouldn't export. By the way, this situation is covered by the LNK4049 and LNK4217 warnings in the Microsoft linker.

Here's an example of a situation where you might get accidental incorrect imports: https://gcc.godbolt.org/z/VAl5Kn

void g(int);
template <typename T>
struct Foo {
  static void f() { g(sizeof(T)); }
};
// Uncomment Bar to see Foo<int>::f get imported
//struct __declspec(dllimport) Bar : Foo<int> {};
void h() { Foo<int>::f(); }

I don't think it would be correct to upgrade Foo<int>::f to dllexport.

ormris updated this revision to Diff 178784.Dec 18 2018, 2:20 PM

That makes sense. Here's an updated diff.

Changelog:

  • Remove dllimport instead of replacing it.
  • Remove unnecessary test change.
ormris edited the summary of this revision. (Show Details)Dec 18 2018, 2:21 PM
ormris retitled this revision from [ThinLTO] Change locally defined symbols marked dllimport to dllexport to [ThinLTO] Remove dllimport attribute from locally defined symbols.
rnk added inline comments.Dec 18 2018, 3:06 PM
include/llvm/IR/GlobalValue.h
198–199

This change doesn't seem quite right. On the one hand, if a GlobalValue has internal linkage, it probably shouldn't be marked dllimport. However, we really shouldn't look at visibility or external weak linkage, which are concepts that don't exist for COFF. And we probably shouldn't adjust the dll storage class from a function named maybeSetDsoLocal.

lib/LTO/LTO.cpp
696–698

This looks like the important functional change. Would the test still pass if you reverted the changes other than this one?

ormris updated this revision to Diff 178818.Dec 18 2018, 5:10 PM

I've trimmed the patch, as suggested.

ormris marked an inline comment as done.Dec 18 2018, 5:10 PM
rnk accepted this revision.Dec 19 2018, 9:59 AM

lgtm, thanks!

This revision is now accepted and ready to land.Dec 19 2018, 9:59 AM

Thanks for the review! I'll go ahead and commit this.

This revision was automatically updated to reflect the committed changes.