Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark a function or global variable as dso_local if possible #3713

Closed
wants to merge 1 commit into from

Conversation

kubo39
Copy link
Contributor

@kubo39 kubo39 commented May 3, 2021

dso_local allows the compiler to assume a function or global variable will resolve to a symbol within the same linkage unit.

reference: https://llvm.org/docs/LangRef.html#runtime-preemption-specifiers

@JohanEngelen
Copy link
Member

See #2783

@JohanEngelen
Copy link
Member

Is dso_local correct in all cases?

  • weak functions
  • with all relocation models
  • DLL export
  • ...?

@kubo39
Copy link
Contributor Author

kubo39 commented May 3, 2021

@JohanEngelen

Is dso_local correct in all cases?

It depends on binary format(i.e,` on COFF, extern_weak or dllimport GlobalValues must be dso_preemptable), These rules have been implemented in LLVM(shouldAssumeDSOLocal function) and I use it.

My concern is that LLVM 12 drops implied dso_local for definitions in ELF static relocation model/PIE to avoid copy relocation.
Clang/Rust implemented the original shouldAssumeDSOLocal functions which implies dso_local in such case, I'm not sure it'd better in our case.

@JohanEngelen
Copy link
Member

Please add testcases for all known interesting cases. Then automatically we will notice when LLVM's behavior is changing.

@kubo39 kubo39 marked this pull request as draft May 3, 2021 11:07
@kubo39 kubo39 force-pushed the dso_local branch 2 times, most recently from b6af27a to b3839ee Compare May 4, 2021 09:18

// CHECK: declare extern_weak void @weakreffunction
pragma(LDC_extern_weak) extern(C) void weakreffunction();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -0,0 +1,5 @@
// REQUIRES: atleast_llvm1200
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what makes this test LLVM>=12? Is the behavior still correct for <12 ?

@JohanEngelen
Copy link
Member

For the testing, I think it is more clear if you put all possibilities in one file, and have multiple //RUN lines with different triples and flags, and distinguish between the different cases using a prefix for FileCheck. That way, it is easier to see that the testing is complete and what the behavior differences are (and should be) for different triples.

dso_local allows the compiler to assume a function or global variable
will resolve to a symbol within the same linkage unit.

reference: https://llvm.org/docs/LangRef.html#runtime-preemption-specifiers

import ldc.attributes : weak;

// ELF_STATIC_RELOC: define{{( dso_local)?}} i32 @{{.*}}3foo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want dso_local here and in the other cases? (not optional)

@weak int baz() { return 42; }


version(Windows)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the version(Windows) ? This should be tested for all triples. The extern_weak attribute is afaik only useful for ELF object files.

// MACHO_STATIC_RELOC: define dso_local i32 @{{.*}}3foo
// MACHO_PIC: define dso_local i32 @{{.*}}3foo
// COFF: define dso_local {{.*}} i32 @{{.*}}3foo
private int foo() { return 42; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides functions, can you also add global variables? (@weak is also applicable to global variables btw)

@kinke
Copy link
Member

kinke commented May 5, 2021

I doubt this has any real effect, because the spec talks about

Direct access will be generated even if the definition is not within this compilation unit.

but the attribute is currently set for definitions only. So I think the interesting case is the opposite, declarations in other IR modules.

It most likely also won't affect Windows at all, because the loader doesn't support such runtime preemption AFAIK.

Whether these direct accesses are safe is another question; TypeInfos and instantiated symbols might be defined in multiple shared objects (exe/.so), so identity checks relying on the loader uniquing such symbols will probably break. AFAIK, that's why MSVC++ EH is based on type strings, and why comparing a TypeInfo will probably have to be based on string comparisons as well - e.g., I think this accounts for the last remaining failure in #3704.

@kubo39
Copy link
Contributor Author

kubo39 commented May 13, 2021

Ah,, true. I'm wrong so closing now., sorry for noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants