In D122215#3998105, @pmatos wrote:

In D122215#3991648, @MoritzS wrote:

Thanks for the patch! I just tried it out and I think this enables many interesting use cases for WebAssembly when using C/C++.

Currently the lowering to wasm does not work when using externrefs when compiling without optimizations. Is that intended behavior?

I have posted a new patch that fixed this. Can you please try it again and see if it works as you expect?

Thanks for the patch! I just tried it out and I think this enables many interesting use cases for WebAssembly when using C/C++.

To answer your questions in the comments about what to do about resolvers with arguments: At least glibc always calls ifunc resolvers without any arguments. It just reads the address of the resolver function from the ELF file, casts it to a pointer to a void-returning function with no arguments and calls it.

The only failing "test" is clang-format. Should I reformat the complete files? Because my additions are formatted correctly, but the rest of the file is not.

Use llvm::cantFail instead of just returning

Fixed an unchecked Expected<T> which lead to several test failures. All tests should run now, hopefully.

Fixed a failing test: In ELFObjectFile I added a case to set the symbol type to
ST_Function also for ifunc symbols. As far as I know, ifunc symbols can only be
functions, but even objdump from binutils doesn't print those symbols with the
function type. I removed that case again, ifunc symbols now have type ST_Other.

Rebased onto main, let tests run again

@lhames do you have any other comments? Can I merge this?

Rebased again because of failing tests on main

Rebased onto main

Fixed typos in comments

Removed Indirect from JITSymbolFlags

In D105465#3653761, @lhames wrote:

Does the Indirect flag really need to go onto JITSymbolFlags? My mental model of ifuncs (very limited -- I haven't used them myself) is "self-updating PLT" -- its ifunc-ness is opaque to callers right?

The test failures all seem to be unrelated to this change, lgtm.

In D105466#3107759, @brad wrote:

This commit appears to have broken the build on OpenBSD..

ld: error: undefined symbol: LLVMRTDyldTLSSpace

referenced by llvm-rtdyld.cpp:372 (/home/brad/llvm/llvm-new/llvm/tools/llvm-rtdyld/llvm-rtdyld.cpp:372)

In D109293#2985435, @MoritzS wrote:

In D109293#2985202, @lhames wrote:

In D109293#2984957, @MoritzS wrote:

In general I would suggest not trying to resolve a TLSGD/LD relocation at all but instead converting it to a GOTTPOFF relocation. Then, no runtime function to implement __tls_get_addr is needed.

Is that compatible with adding extra code at runtime? I suspect we'll need to go the other way: convert things to function calls by default, but make it possible to use direct models where they're safe. That's speculation having not read the TLS spec though.

That depends on how the allocation of TLS sections is implemented, but in general you are right, I didn't think about that.

The problem I faced when implementing the TLS relocations is that we can't chose which types of TLS relocations the object files we try to link use. This means that we need to implement the GOTTPOFF/TPOFF relocations that do not go through the indirection of __tls_get_addr. They just resolve to an offset that will be added to the value stored in %fs:0. This is usually set initially by ld.so when a process starts by using arch_prctl(ARCH_SET_FS), or for newer CPUS by using the wrfsbase instruction. This means that we can't easily extend the storage of the process that runs the runtime linker to add new TLS sections since messing with the fs register will very likely lead to unexpected results. There are several possible solutions for that with different disadvantages:

1. Don't allow GOTTPOFF/TPOFF relocations at all. This means that we will not be able to link many pre-compiled static libraries.
2. Allow all TLS relocations. Since we can't add new TLS storage at runtime, define a global thread-local variable which will then be used by the linker to resolve GOTTPOFF/TPOFF relocations.
3. Rewrite the relocated code so that it does not use the fs register. On Linux we could probably replace all references to fs by gs and then manually set the gs register? I'm not sure if this can break code if we change instructions that did not originally have a relocation.

I implemented 2. in llvm-rtdyld. Since this is mainly used for testing, the size of the pre-allocated TLS section is only 16 B. In a "real" program you could easily allocate an entire page without noticing any runtime overhead. This is more than enough to link an entire static glibc, for example.

In D109293#2985202, @lhames wrote:

In D109293#2984957, @MoritzS wrote:

In general I would suggest not trying to resolve a TLSGD/LD relocation at all but instead converting it to a GOTTPOFF relocation. Then, no runtime function to implement __tls_get_addr is needed.

Is that compatible with adding extra code at runtime? I suspect we'll need to go the other way: convert things to function calls by default, but make it possible to use direct models where they're safe. That's speculation having not read the TLS spec though.

In D109293#2984338, @lhames wrote:

To allocate the TLS descriptor in GOT, I need to get the edge kind information in PerGraphGOTAndPLTStubBuilder, So I add a Edge::Kind K argument in some functions in PerGraphGOTAndPLTStubBuilder.h. If it is not
suitable, I can think further to solve this problem.

Is there a good reason to put these in the GOT? I would create and manage a different section for these, rather than re-using the GOT.

In D105466#2984334, @lhames wrote:

Ok. I've just realized that there's also a danger on the flip-side too: code containing TLVs will be accepted, but this only support accessing them on one thread, which will be surprising to some clients. I think it would be worth adding an "enablePartialTLVSupport" method to the memory manager (which should default to false), and gating this new feature on that. If you're happy to do that before the code lands then that's great, otherwise I'm happy to do it in-tree after it lands.

Added NOLINT comments to avoid clang-tidy warning for the functions called that
have X86_64 in their name.

Require x86_64-linux for the TLS test as the execution in llvm-rtdyld is
ifdef'd only for x86_64 and ELF.

Refactored most of the relocation logic into separate functions.

In D105466#2968873, @lhames wrote:

What was the previous behavior when we encountered these relocations?

My only concern here is avoiding regressing existing users if we turn a previously unsupported (but silently accepted) relocation into a potential link-time error. It's perverse, but this kind of improvement to functionality and error checking has caused problems for clients before, and the main aim for MCJIT / RuntimeDyld right now is stability.

If the new support could cause a problem then I think the best solution would be to add a 'bool supportsTLV' method to the memory manager that we can query before running any of the TLV code.

Thanks for looking at this!

User::operator delete(Ptr) must not be called with any additional arguments. Fixed that.

Fixed variable names

This is now solved in D97898.

Rebased onto main

Rebased onto main

As part of a research project on a code-generating database system I am using LLVM to generate code and also link it with static libraries at runtime. I found a few bugs in RuntimeDyld most of which I submitted for a review. The biggest change that I haven't submitted yet is my implementation of TLS relocations (only for x86 for now) in RuntimeDyld. I know that long-term this will probably be replaced by JITLink but when I started working on that project, it didn't seem stable enough so I decided to extend RuntimeDyld.

ping

Rebased onto master

Some tests were failing, I'll try to fix that and then commit the changes.

Initialize bool flags

Rebased onto master, run tests again

In D94475#2493513, @jrtc27 wrote:

Looking at the original motivation for the patch I'm not convinced that llvm-readelf etc shouldn't just fall back on displaying the "normal" set of GNU extensions even for ELFOSABI_NONE (i.e. those that FreeBSD and GNU have in common). Things like that are really just part of de-facto generic ELF these days. What does binutils do?

In D94475#2493461, @MaskRay wrote:

This is known. GNU as by default emits ELFOSABI_NONE on Linux - if it does not use any GNU extensioin. If the assembly uses STT_GNU_IFUNC/STB_GNU_UNIQUE, GNU as will switch to ELFOSABI_GNU.

The patch improves the few cases but regresses other cases, so I am requesting changes. It is also unclear whether respecting the GNU behavior is strictly necessary (see gold https://sourceware.org/bugzilla/show_bug.cgi?id=17735). STT_GNU_IFUNC is GNU extension and it is adopted by some other OSes. I am not sure the readelf -S output is a justifying reason for this change.

Fixed clang-format error

Thanks for the review! Can you commit this for me, please?

Rebased onto master

Rebased onto master

ping

Another way to avoid UB in that case is to use value initialization, i.e. PathDiagnosticConsumerOptions options{};.

Rebased onto master

Can you commit this please? I don't have commit access.

Read tombstone value from a constexpr variable. The getTombstoneVal() function
is still necessary, though, as reinterpret_cast is not a constant expression.

• Added test case
• Use generator instead of iterator class

Rebased onto master

I don't have commit access, so please go ahead and commit it.

ping

Fixed build error

Yes, please commit this.