Page MenuHomePhabricator

MoritzS (Moritz Sichert)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 13 2017, 4:08 AM (232 w, 3 d)

Recent Activity

Feb 17 2022

MoritzS accepted D119991: [RuntimeDyld] Fix building on OpenBSD.

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

Feb 17 2022, 2:57 AM · Restricted Project

Nov 16 2021

MoritzS added a comment to D105466: [RuntimeDyld] Implemented relocation of TLS symbols in ELF.

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)

Nov 16 2021, 1:39 AM · Restricted Project

Sep 8 2021

MoritzS added inline comments to D109293: [JITLink] Add initial native TLS support to ELFNix platform.
Sep 8 2021, 2:53 AM · Restricted Project, Restricted Project

Sep 6 2021

MoritzS added a comment to D109293: [JITLink] Add initial native TLS support to ELFNix platform.

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.

Sep 6 2021, 8:40 AM · Restricted Project, Restricted Project
MoritzS added a comment to D109293: [JITLink] Add initial native TLS support to ELFNix platform.

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.

Sep 6 2021, 8:28 AM · Restricted Project, Restricted Project
MoritzS added a comment to D109293: [JITLink] Add initial native TLS support to ELFNix platform.

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.

Sep 6 2021, 3:27 AM · Restricted Project, Restricted Project
MoritzS added a comment to D105466: [RuntimeDyld] Implemented relocation of TLS symbols in ELF.

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.

Sep 6 2021, 1:37 AM · Restricted Project
MoritzS committed rGa0a596449981: [RuntimeDyld] Implemented relocation of TLS symbols in ELF (authored by MoritzS).
[RuntimeDyld] Implemented relocation of TLS symbols in ELF
Sep 6 2021, 1:31 AM
MoritzS committed rGf6873786034a: [RuntimeDyld] Implemented relocation for ELF::R_X86_64_GOTPC32 (authored by MoritzS).
[RuntimeDyld] Implemented relocation for ELF::R_X86_64_GOTPC32
Sep 6 2021, 1:31 AM
MoritzS closed D105466: [RuntimeDyld] Implemented relocation of TLS symbols in ELF.
Sep 6 2021, 1:31 AM · Restricted Project
MoritzS closed D95512: [RuntimeDyld] Implemented relocation for ELF::R_X86_64_GOTPC32.
Sep 6 2021, 1:31 AM · Restricted Project

Aug 29 2021

MoritzS updated the diff for D105466: [RuntimeDyld] Implemented relocation of TLS symbols in ELF.

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

Aug 29 2021, 11:51 PM · Restricted Project

Aug 28 2021

MoritzS updated the diff for D105466: [RuntimeDyld] Implemented relocation of TLS symbols in ELF.

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

Aug 28 2021, 3:39 AM · Restricted Project

Aug 27 2021

MoritzS updated the diff for D105466: [RuntimeDyld] Implemented relocation of TLS symbols in ELF.

Refactored most of the relocation logic into separate functions.

Aug 27 2021, 8:51 AM · Restricted Project
MoritzS added inline comments to D105466: [RuntimeDyld] Implemented relocation of TLS symbols in ELF.
Aug 27 2021, 6:34 AM · Restricted Project
MoritzS added a comment to D105466: [RuntimeDyld] Implemented relocation of TLS symbols in ELF.

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.

Aug 27 2021, 6:33 AM · Restricted Project

Aug 18 2021

MoritzS added a comment to D105466: [RuntimeDyld] Implemented relocation of TLS symbols in ELF.

Thanks for looking at this!

Aug 18 2021, 1:38 AM · Restricted Project

Jul 8 2021

MoritzS committed rGd58c7a92380e: [IR] Added operator delete to subclasses of User to avoid UB (authored by MoritzS).
[IR] Added operator delete to subclasses of User to avoid UB
Jul 8 2021, 3:00 AM
MoritzS closed D103143: [IR] Added operator delete to subclasses of User to avoid UB.
Jul 8 2021, 3:00 AM · Restricted Project
MoritzS updated the diff for D103143: [IR] Added operator delete to subclasses of User to avoid UB.

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

Jul 8 2021, 2:11 AM · Restricted Project
MoritzS updated the diff for D103143: [IR] Added operator delete to subclasses of User to avoid UB.

Fixed variable names

Jul 8 2021, 12:22 AM · Restricted Project

Jul 7 2021

MoritzS added reviewers for D103143: [IR] Added operator delete to subclasses of User to avoid UB: aeubanks, dblaikie.
Jul 7 2021, 3:38 AM · Restricted Project

Jul 6 2021

MoritzS requested review of D105466: [RuntimeDyld] Implemented relocation of TLS symbols in ELF.
Jul 6 2021, 2:10 AM · Restricted Project
MoritzS requested review of D105465: [RuntimeDyld] Added support for relocation of indirect functions.
Jul 6 2021, 1:27 AM · Restricted Project
MoritzS abandoned D95597: [RuntimeDyld] Don't error on symbols that resolve to 0.

This is now solved in D97898.

Jul 6 2021, 12:51 AM · Restricted Project
MoritzS updated the diff for D103143: [IR] Added operator delete to subclasses of User to avoid UB.

Rebased onto main

Jul 6 2021, 12:45 AM · Restricted Project
MoritzS updated the diff for D95512: [RuntimeDyld] Implemented relocation for ELF::R_X86_64_GOTPC32.

Rebased onto main

Jul 6 2021, 12:40 AM · Restricted Project

May 26 2021

MoritzS requested review of D103143: [IR] Added operator delete to subclasses of User to avoid UB.
May 26 2021, 2:15 AM · Restricted Project

Apr 26 2021

MoritzS added a comment to D95596: [RuntimeDyld] Fixed buffer overflows with absolute symbols.

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.

Apr 26 2021, 10:28 AM · Restricted Project
MoritzS committed rG10038d0b3dfc: [RuntimeDyld] Fixed buffer overflows with absolute symbols (authored by MoritzS).
[RuntimeDyld] Fixed buffer overflows with absolute symbols
Apr 26 2021, 10:25 AM
MoritzS closed D95596: [RuntimeDyld] Fixed buffer overflows with absolute symbols.
Apr 26 2021, 10:24 AM · Restricted Project

Apr 14 2021

MoritzS added a comment to D95512: [RuntimeDyld] Implemented relocation for ELF::R_X86_64_GOTPC32.

ping

Apr 14 2021, 12:48 AM · Restricted Project

Feb 18 2021

MoritzS updated the diff for D95512: [RuntimeDyld] Implemented relocation for ELF::R_X86_64_GOTPC32.

Rebased onto master

Feb 18 2021, 3:09 AM · Restricted Project

Jan 28 2021

MoritzS requested review of D95597: [RuntimeDyld] Don't error on symbols that resolve to 0.
Jan 28 2021, 1:05 AM · Restricted Project
MoritzS requested review of D95596: [RuntimeDyld] Fixed buffer overflows with absolute symbols.
Jan 28 2021, 1:03 AM · Restricted Project

Jan 27 2021

MoritzS requested review of D95512: [RuntimeDyld] Implemented relocation for ELF::R_X86_64_GOTPC32.
Jan 27 2021, 12:30 AM · Restricted Project

Jan 22 2021

MoritzS committed rGb46545542b30: Avoid fragile type lookups in GDB pretty printer (authored by MoritzS).
Avoid fragile type lookups in GDB pretty printer
Jan 22 2021, 5:58 AM
MoritzS closed D94431: Avoid fragile type lookups in GDB pretty printer.
Jan 22 2021, 5:57 AM · Restricted Project
MoritzS committed rGe16959c9b855: Don't delete default constructor of PathDiagnosticConsumerOptions (authored by MoritzS).
Don't delete default constructor of PathDiagnosticConsumerOptions
Jan 22 2021, 4:47 AM
MoritzS closed D92221: Don't delete default constructor of PathDiagnosticConsumerOptions.
Jan 22 2021, 4:47 AM · Restricted Project
MoritzS added a comment to D92221: Don't delete default constructor of PathDiagnosticConsumerOptions.

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

Jan 22 2021, 3:05 AM · Restricted Project
MoritzS updated the diff for D92221: Don't delete default constructor of PathDiagnosticConsumerOptions.

Initialize bool flags

Jan 22 2021, 2:59 AM · Restricted Project
MoritzS updated the diff for D92221: Don't delete default constructor of PathDiagnosticConsumerOptions.

Rebased onto master, run tests again

Jan 22 2021, 1:51 AM · Restricted Project

Jan 12 2021

MoritzS added a comment to D94475: [MC] Emit ELF files with ELFOSABI_GNU if specified in triple.

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?

Jan 12 2021, 10:38 AM · Restricted Project
MoritzS added a comment to D94475: [MC] Emit ELF files with ELFOSABI_GNU if specified in triple.

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.

Jan 12 2021, 10:36 AM · Restricted Project
MoritzS updated the diff for D94475: [MC] Emit ELF files with ELFOSABI_GNU if specified in triple.

Fixed clang-format error

Jan 12 2021, 2:10 AM · Restricted Project
MoritzS added inline comments to D94475: [MC] Emit ELF files with ELFOSABI_GNU if specified in triple.
Jan 12 2021, 2:07 AM · Restricted Project
MoritzS requested review of D94475: [MC] Emit ELF files with ELFOSABI_GNU if specified in triple.
Jan 12 2021, 1:44 AM · Restricted Project
MoritzS added a comment to D94431: Avoid fragile type lookups in GDB pretty printer.

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

Jan 12 2021, 12:35 AM · Restricted Project

Jan 11 2021

MoritzS requested review of D94431: Avoid fragile type lookups in GDB pretty printer.
Jan 11 2021, 9:55 AM · Restricted Project

Dec 21 2020

MoritzS updated the diff for D92221: Don't delete default constructor of PathDiagnosticConsumerOptions.

Rebased onto master

Dec 21 2020, 10:26 AM · Restricted Project
MoritzS updated the diff for D89373: Fixed dangling reference in RuntimeDyldELF.

Rebased onto master

Dec 21 2020, 10:23 AM · Restricted Project

Dec 10 2020

MoritzS added a comment to D92221: Don't delete default constructor of PathDiagnosticConsumerOptions.

ping

Dec 10 2020, 3:19 AM · Restricted Project

Nov 30 2020

MoritzS added a comment to D92221: Don't delete default constructor of PathDiagnosticConsumerOptions.

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

Nov 30 2020, 12:20 AM · Restricted Project

Nov 27 2020

MoritzS requested review of D92221: Don't delete default constructor of PathDiagnosticConsumerOptions.
Nov 27 2020, 3:35 AM · Restricted Project

Nov 26 2020

MoritzS updated the diff for D89373: Fixed dangling reference in RuntimeDyldELF.

Rebased onto master

Nov 26 2020, 12:47 AM · Restricted Project

Nov 18 2020

MoritzS added a comment to D91183: Added GDB pretty printer for StringMap.

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

Nov 18 2020, 12:19 AM · Restricted Project

Nov 17 2020

MoritzS added inline comments to D91183: Added GDB pretty printer for StringMap.
Nov 17 2020, 11:22 AM · Restricted Project
MoritzS updated the diff for D91183: Added GDB pretty printer for StringMap.

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

Nov 17 2020, 11:21 AM · Restricted Project

Nov 12 2020

MoritzS added inline comments to D91183: Added GDB pretty printer for StringMap.
Nov 12 2020, 1:11 AM · Restricted Project

Nov 10 2020

MoritzS added inline comments to D91183: Added GDB pretty printer for StringMap.
Nov 10 2020, 12:23 PM · Restricted Project
MoritzS updated the diff for D91183: Added GDB pretty printer for StringMap.
  • Added test case
  • Use generator instead of iterator class
Nov 10 2020, 12:21 PM · Restricted Project
MoritzS requested review of D91183: Added GDB pretty printer for StringMap.
Nov 10 2020, 9:21 AM · Restricted Project
MoritzS updated the diff for D89373: Fixed dangling reference in RuntimeDyldELF.

Rebased onto master

Nov 10 2020, 4:11 AM · Restricted Project

Nov 5 2020

MoritzS added a comment to D89373: Fixed dangling reference in RuntimeDyldELF.

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

Nov 5 2020, 10:31 AM · Restricted Project

Nov 3 2020

MoritzS added a reviewer for D89373: Fixed dangling reference in RuntimeDyldELF: lhames.
Nov 3 2020, 5:50 AM · Restricted Project

Oct 26 2020

MoritzS added a comment to D89373: Fixed dangling reference in RuntimeDyldELF.

ping

Oct 26 2020, 12:22 PM · Restricted Project

Oct 14 2020

MoritzS updated the diff for D89373: Fixed dangling reference in RuntimeDyldELF.

Fixed build error

Oct 14 2020, 1:55 AM · Restricted Project
MoritzS requested review of D89373: Fixed dangling reference in RuntimeDyldELF.
Oct 14 2020, 1:47 AM · Restricted Project

Dec 22 2017

MoritzS added a comment to D41170: Use dbgs() instead of errs() in docs for DEBUG().

Yes, please commit this.

Dec 22 2017, 4:19 AM

Dec 13 2017

MoritzS created D41170: Use dbgs() instead of errs() in docs for DEBUG().
Dec 13 2017, 4:30 AM