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

fix linker scripts with thread section #542

Merged
merged 1 commit into from
Feb 6, 2023
Merged

Conversation

palainp
Copy link
Contributor

@palainp palainp commented Jan 19, 2023

Hi, this PR should fix the issue of missing thread data section in the linking step (see mirage/ocaml-solo5#124 (comment)). It works (compiles + links) on a 12.2 FreeBSD as well as in my linux fedora AppVM.

I haven't added yet other than spt+hvt, not sure what is needed (maybe xen too to compiles for Xen/Qubes with llvm) and this PR should trigger a CI test to make sure that I haven't broken something.

As a comment it just add a PT_TLS section in the ELF binary and the linker is able to merge every TLS bits. The tbss part is also now after bss to respect the order of the data and tdata sections but that should not be mandatory.

@palainp
Copy link
Contributor Author

palainp commented Jan 19, 2023

Still TBD: protect the TLS section and update

__attribute__((always_inline)) static inline void crt_init_tls(void)
to point to that section (for our single code solo5).

@palainp
Copy link
Contributor Author

palainp commented Jan 19, 2023

All scripts needed to be modified for the test. Not sure why the CI fails with OpenBSD, maybe the clang version is different?

@hannesm
Copy link
Contributor

hannesm commented Jan 19, 2023

Thanks for your work and PR. Why has the tbss been moved after bss?

@palainp
Copy link
Contributor Author

palainp commented Jan 19, 2023

There is no fundamental reason, except keeping the same order as for the data and tdata section. I guess it's ok to move it back before bss.

@hannesm
Copy link
Contributor

hannesm commented Jan 19, 2023

I'm not saying we should not do that, I was just curious whether there was an underlying technical reason. I agree that having the same order as data / tdata makes sense. :)

@dinosaure
Copy link
Collaborator

It seems that this PR fixes our initial issue on ocaml-solo5 about FreeBSD but we still the OpenBSD support. May be @adamsteen has some ideas about the TLS support?

@adamsteen
Copy link
Contributor

adamsteen commented Jan 21, 2023 via email

@palainp
Copy link
Contributor Author

palainp commented Jan 21, 2023

I tried to compile on OpenBSD 7.2, not sure if that's the same issue, as I can't see the CI logs, but on my laptop, when building an hvt unikernel, it complains about __emutls_get_address not available at the linking step.

A quick search shows that it should be exported on OpenBSD with compiler-rt (ziglang/zig#5921 (comment)) but unsure that this is related to our case. Maybe WE need some specific arguments in the toolchain?

@palainp
Copy link
Contributor Author

palainp commented Jan 30, 2023

Happy, not happy, putting -fno-emulated-tls in TARGET_CC_CFLAGS in configure.sh to create the OpenBSD toolchain scripts seems to pass the test (I'll push a commit as soon as I'll get my laptop back).

Sadly I was unable to run an hvt hello unikernel (page fault in domain_create, exactly the same as in mirage/ocaml-solo5#122 (comment) , with a pin to the ocaml-solo5#500-cleaned branch, I'll soon try to gdb it to see what is the %fs register value there).

@hannesm
Copy link
Contributor

hannesm commented Jan 31, 2023

Dear @palainp, thanks for this PR.

We need the following to actually execute the tls tests:

diff --git a/tests/tests.bats b/tests/tests.bats
index d911eb0..c542b7a 100644
--- a/tests/tests.bats
+++ b/tests/tests.bats
@@ -416,15 +416,11 @@ xen_expect_abort() {
 }
 
 @test "tls hvt" {
-  skip_unless_host_is Linux
-
   hvt_run test_tls/test_tls.hvt
   expect_success
 }
 
 @test "tls virtio" {
-  skip_unless_host_is Linux # XXX is this necessary for virtio?
-
   virtio_run test_tls/test_tls.virtio
   virtio_expect_success
 }

And, at least on my FreeBSD laptop, the test_tls fails:

 ✗ tls hvt
   (from function `expect_success' in file tests.bats, line 165,
    in test file tests.bats, line 420)
     `expect_success' failed
               |      ___|
     __|  _ \  |  _ \ __ \
   \__ \ (   | | (   |  ) |
   ____/\___/ _|\___/____/
   Solo5: Bindings version v0.7.5-7-g69a894b
   Solo5: Memory map: 2 MB addressable:
   Solo5:   reserved @ (0x0 - 0xfffff)
   Solo5:       text @ (0x100000 - 0x104fff)
   Solo5:     rodata @ (0x105000 - 0x105fff)
   Solo5:       data @ (0x106000 - 0x10afff)
   Solo5:       heap >= 0x10b000 < stack < 0x200000
   
   **** Solo5 standalone test_tls ****
   
   Solo5: trap: type=#PF ec=0x0 rip=0x104c44 rsp=0x1fffc0 rflags=0x10046
   Solo5: trap: cr2=0x2
   Solo5: ABORT: cpu_x86_64.c:181: Fatal trap

According to objdump, looking at 4c44:

0000000000104c40 <get_data>:
  104c40:       55                      push   %rbp
  104c41:       48 89 e5                mov    %rsp,%rbp
  104c44:       64 48 8b 04 25 00 00    mov    %fs:0x0,%rax
  104c4b:       00 00 
  104c4d:       48 8b 80 f0 ff ff ff    mov    -0x10(%rax),%rax
  104c54:       5d                      pop    %rbp
  104c55:       c3                      ret    
  104c56:       66 2e 0f 1f 84 00 00    cs nopw 0x0(%rax,%rax,1)
  104c5d:       00 00 00 

So, it looks like we need some setup to avoid the page fault... Does this make sense? Does anyone know what needs to be done?

@palainp
Copy link
Contributor Author

palainp commented Jan 31, 2023

Oups you're totally right ! :ashamed:

I don't understand why, but the set_data(2) call (

set_data(2);
) somehow overwrites tcb1.tp and leaves us with the wrong TLS address after that. There is something to investigate here as it seems we only have one __thread uint64_t variable and the struct size should be sufficient.

And investigating this point leads me to a possible bug in ocaml-solo5, we currently point TLS to the first address in the TLS zone, and it should point to the last address, this is consistent with the offset I can see in the assembly being negative, am I right?

- add sections with PT_TLS property
- lld seems to need all TLS sections defined in header
- remove the stub in test_tls.c for clang compiler suite
@palainp
Copy link
Contributor Author

palainp commented Feb 5, 2023

So the issue was due to an alignment difference for some targets (for example, the data section was a bit larger with hvt than with spt target, and the tbss section need to be aligned differently). It's fixed now by tellin lld that tbss is also a TLS related section with a correct test_tls.c code.
The usage for those TLS related things should be as in the test file:

  • define a struct as variant I or II depending on the processor, like for variant II:
struct tls {
    void* _data;
    void* pt;
};
  • allocate enough [1] memory in the _data field
  • set the tp field to it's address tls.tp = &tls.tp;
  • call solo5_set_tls_base(tls.tp);

[1]: in order to be able to do that, we probably need to add some symbol in the linker scripts

@palainp
Copy link
Contributor Author

palainp commented Feb 5, 2023

And if we want to have multiple threads (solo5 multicore or for the main thread in ocaml-solo5), we need to to that for each one. It seems easier to use dynamic allocation for the allocating part of the procedure as the size of the TLS can be computed only in the link pass.

@hannesm
Copy link
Contributor

hannesm commented Feb 6, 2023

Thanks again @palainp. I have one small question, though -- do you understand why with the earlier commit (where there was no tbss PT_TLS in any linker script) there was a difference in behaviour between spt and hvt?

@hannesm
Copy link
Contributor

hannesm commented Feb 6, 2023

I have tested this on my FreeBSD 13.1 laptop successfully, thus approving.

@dinosaure any chance you get around to merge this and #544 and cut a release? This will help us to unblock the ocaml-solo5 OCaml 5 PR.

@dinosaure
Copy link
Collaborator

Yes, I will take a look in them deeply today and unlock the release process.

@dinosaure
Copy link
Collaborator

I found this description about PT_TLS which corresponds to the alignement story explained by @palainp:

TLS variables are much like any other global/static variable. In implementation their initial data winds up in the PT_TLS segment. The PT_TLS segment is inside of a read only PT_LOAD segment despite TLS variables being writable. This segment is then copied into the process for each thread in a unique writable location. The location the PT_TLS segment is copied to is influenced by the segment's alignment to ensure that the alignment of TLS variables is respected.

@dinosaure dinosaure merged commit a63a755 into Solo5:master Feb 6, 2023
@palainp palainp deleted the add-shf branch February 6, 2023 15:57
@palainp palainp mentioned this pull request Feb 15, 2023
dinosaure added a commit that referenced this pull request Apr 25, 2023
According to #542 & #546, we change the memory layout of unikernels (and added
sections needed for the TLS support). We must assert an incompatibility between
solo5.0.7.* and solo5.0.8.0 and be sure that old tenders will not be used with
new built unikernels.

Co-authored-by: Hannes Mehnert <[email protected]>
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Apr 25, 2023
* Be able to build `spt`, `virtio`, `muen` and `xen` targets on OpenBSD
  (@adamsteen, Solo5/solo5#544). This change does not allow us to "run" these
  targets on OpenBSD
* Fix linker scripts with TLS (Thread Local Storage) sections
  (@palainp, @hannesm, @dinosaure, Solo5/solo5#542)
* Export TLS symbols (@palainp, @hannesm, @dinosaure, Solo5/solo5#546)
  **breaking change** due to Solo5/solo5#542 & Solo5/solo5#546, tenders must be
  **upgraded**. Indeed, solo5.0.7.* tenders will not be able to load correctly
  unikernels compiled with solo5.0.8.0. The internal ABI version for
  `solo5-hvt`/`solo5-spt` was upgraded accordingly.

  This version implements Thread Local Storage. The user can initialise a TLS
  block with `solo5_tls_init` on a pointer to `solo5_tls_size()` free bytes.
  Then, the user is able to set the `tp` (Thread Pointer) pointer via
  `solo5_set_tls_base(solo5_tls_tp_offset(tls_block))`. More details are
  available into `solo5.h`.

  Note: this change does not allow a Solo5 unikernel to use multiple cores! It
  only provides an API required by OCaml 5 / pthread to launch, at most, one
  thread.
* Fix tests reported by NixOS (@greydot, @ehmry, Solo5/solo5#547)
* Split out the `time.c` implementation between Muen and HVT
  (@dinosaure, @Kensan, Solo5/solo5#552)
* User hypercall instead of TSC-based clock when the user asks for the
  wall-clock (@dinosaure, @reynir, Solo5/solo5#549, Solo5/solo5#550)

  Note: only hvt & virtio are updated to avoid a clock drift on the wall-clock.
  Indeed, when the unikernel is suspended, the wall-clock is not updated. Muen
  & Xen still use a TSC-based wall-clock. The spt target was already in sync
  with the host's wall-clock.
* Improve the muen clock (@Kensan, Solo5/solo5#553)
* Fix the `.bss` section according to Solo5/solo5#542 & Solo5/solo5#546. The
  `.bss` section is tagged with `PT_LOAD`. Tenders are available to load this
  section properly. (@Kensan, @dinosaure, Solo5/solo5#551, Solo5/solo5#554)
* Fix the cross-compilation of Solo5 for `aarch64`
  (@dinosaure, @palainp, @hannes, Solo5/solo5#555)
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Apr 25, 2023
  (@adamsteen, Solo5/solo5#544). This change does not allow us to "run" these
  targets on OpenBSD
* Fix linker scripts with TLS (Thread Local Storage) sections
  (@palainp, @hannesm, @dinosaure, Solo5/solo5#542)
* Export TLS symbols (@palainp, @hannesm, @dinosaure, Solo5/solo5#546)
  **breaking change** due to Solo5/solo5#542 & Solo5/solo5#546, tenders must be
  **upgraded**. Indeed, solo5.0.7.* tenders will not be able to load correctly
  unikernels compiled with solo5.0.8.0. The internal ABI version for
  `solo5-hvt`/`solo5-spt` was upgraded accordingly.

  This version implements Thread Local Storage. The user can initialise a TLS
  block with `solo5_tls_init` on a pointer to `solo5_tls_size()` free bytes.
  Then, the user is able to set the `tp` (Thread Pointer) pointer via
  `solo5_set_tls_base(solo5_tls_tp_offset(tls_block))`. More details are
  available into `solo5.h`.

  Note: this change does not allow a Solo5 unikernel to use multiple cores! It
  only provides an API required by OCaml 5 / pthread to launch, at most, one
  thread.
* Fix tests reported by NixOS (@greydot, @ehmry, Solo5/solo5#547)
* Split out the `time.c` implementation between Muen and HVT
  (@dinosaure, @Kensan, Solo5/solo5#552)
* User hypercall instead of TSC-based clock when the user asks for the
  wall-clock (@dinosaure, @reynir, Solo5/solo5#549, Solo5/solo5#550)

  Note: only hvt & virtio are updated to avoid a clock drift on the wall-clock.
  Indeed, when the unikernel is suspended, the wall-clock is not updated. Muen
  & Xen still use a TSC-based wall-clock. The spt target was already in sync
  with the host's wall-clock.
* Improve the muen clock (@Kensan, Solo5/solo5#553)
* Fix the `.bss` section according to Solo5/solo5#542 & Solo5/solo5#546. The
  `.bss` section is tagged with `PT_LOAD`. Tenders are available to load this
  section properly. (@Kensan, @dinosaure, Solo5/solo5#551, Solo5/solo5#554)
* Fix the cross-compilation of Solo5 for `aarch64`
  (@dinosaure, @palainp, @hannes, Solo5/solo5#555)
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Apr 28, 2023
* Be able to build `spt`, `virtio`, `muen` and `xen` targets on OpenBSD
  (@adamsteen, Solo5/solo5#544). This change does not allow us to "run" these
  targets on OpenBSD
* Fix linker scripts with TLS (Thread Local Storage) sections
  (@palainp, @hannesm, @dinosaure, Solo5/solo5#542)
* Export TLS symbols (@palainp, @hannesm, @dinosaure, Solo5/solo5#546)
  **breaking change** due to Solo5/solo5#542 & Solo5/solo5#546, tenders must be
  **upgraded**. Indeed, solo5.0.7.* tenders will not be able to load correctly
  unikernels compiled with solo5.0.8.0. The internal ABI version for
  `solo5-hvt`/`solo5-spt` was upgraded accordingly.

  This version implements Thread Local Storage. The user can initialise a TLS
  block with `solo5_tls_init` on a pointer to `solo5_tls_size()` free bytes.
  Then, the user is able to set the `tp` (Thread Pointer) pointer via
  `solo5_set_tls_base(solo5_tls_tp_offset(tls_block))`. More details are
  available into `solo5.h`.

  Note: this change does not allow a Solo5 unikernel to use multiple cores! It
  only provides an API required by OCaml 5 / pthread to launch, at most, one
  thread.
* Fix tests reported by NixOS (@greydot, @ehmry, Solo5/solo5#547)
* Split out the `time.c` implementation between Muen and HVT
  (@dinosaure, @Kensan, Solo5/solo5#552)
* User hypercall instead of TSC-based clock when the user asks for the
  wall-clock (@dinosaure, @reynir, Solo5/solo5#549, Solo5/solo5#550)

  Note: only hvt & virtio are updated to avoid a clock drift on the wall-clock.
  Indeed, when the unikernel is suspended, the wall-clock is not updated. Muen
  & Xen still use a TSC-based wall-clock. The spt target was already in sync
  with the host's wall-clock.
* Improve the muen clock (@Kensan, Solo5/solo5#553)
* Fix the `.bss` section according to Solo5/solo5#542 & Solo5/solo5#546. The
  `.bss` section is tagged with `PT_LOAD`. Tenders are available to load this
  section properly. (@Kensan, @dinosaure, Solo5/solo5#551, Solo5/solo5#554)
* Fix the cross-compilation of Solo5 for `aarch64`
  (@dinosaure, @palainp, @hannesm, Solo5/solo5#555)
* Increase the Muen ABI (2 to 3) due to TLS changes (@Kensan, ocaml#557)
* Support lifecycle management for Muen (@Kensan, ocaml#557)
  The user is able to configure automatic restarting of unikernels that invokes
  `solo5_ext()`
* Fix the `test_fpu` test & ensure the alignment of variables (@Kensan, ocaml#557)
dinosaure added a commit that referenced this pull request Apr 28, 2023
According to #542 & #546, we change the memory layout of unikernels (and added
sections needed for the TLS support). We must assert an incompatibility between
solo5.0.7.* and solo5.0.8.0 and be sure that old tenders will not be used with
new built unikernels.

Co-authored-by: Hannes Mehnert <[email protected]>
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.

4 participants