This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add a simple x86_64 linux loader.
ClosedPublic

Authored by sivachandra on Mar 18 2020, 11:02 PM.

Details

Summary

This adds a very simple loader. This will be extended to a full loader
in future patches. A utility rule to add unittests has been added to
serve us while we are building out the full loader.

Diff Detail

Event Timeline

sivachandra created this revision.Mar 18 2020, 11:02 PM
  • Fix a typo in a test
  • Add two more tests
Harbormaster completed remote builds in B49695: Diff 251279.

Looks pretty good I think. It fails to link when using sanitizers though, they don't go well with -static I think.

libc/loader/linux/x86_64/start.cpp
49

Maybe just while (*env_end_marker) or while (*env_end_marker != nullptr)

libc/test/loader/CMakeLists.txt
64

Is this always run by /bin/sh? I sometimes use a shell (fish) which needs an env keyword before declaring environment variables so I'm not sure it would work here. if not could we do sh -c ${ADD_LOADER_TEST_ENV} ...?

libc/test/loader/linux/args_test.cpp
13

Could be called streq maybe

sivachandra marked 3 inline comments as done.
  • Do not run loader tests as part of check-libc as they are not yet sanitizer ready.
  • Use add_executable with -nostdlib flag instead of calling the linker directly.

Looks pretty good I think. It fails to link when using sanitizers though, they don't go well with -static I think.

That's my mistake I missed removing loader tests from check-libc. I do not think the loader is ready yet to link to sanitizer runtimes so we should not run loader tests on the sanitizer bot.

phosek added a reviewer: dho.Mar 19 2020, 1:02 PM

Remove incorrect/redundant target_compile_options.

The directory structure libc/loader/linux/x86_64/ does not seem very right. A loader can be made architecture agnostic with a little abstraction. I am concerned this will get copied over to x86 and aarch64.

libc/loader/linux/x86_64/CMakeLists.txt
11

This does not seem correct. clang -Wall -Wextra does not warn on calling main.
You could use #pragma GCC diagnostic ignored if they really warned. More so, such a warning would not be emitted in -ffreestanding mode.

Use -ffreestanding instead of -w.

sivachandra marked an inline comment as done.Mar 19 2020, 10:03 PM

The directory structure libc/loader/linux/x86_64/ does not seem very right. A loader can be made architecture agnostic with a little abstraction. I am concerned this will get copied over to x86 and aarch64.

I agree. This patch is really simple, mostly focusing on the infrastructure. For future patches, I intend to cater to both i386 and x86_64, so some sort of abstraction will be incorporated to handle common code.

The directory structure libc/loader/linux/x86_64/ does not seem very right. A loader can be made architecture agnostic with a little abstraction. I am concerned this will get copied over to x86 and aarch64.

I agree. This patch is really simple, mostly focusing on the infrastructure. For future patches, I intend to cater to both i386 and x86_64, so some sort of abstraction will be incorporated to handle common code.

I hope we won't leave that in the future. This can simply be improved now.

abrachet accepted this revision.Mar 23 2020, 9:18 PM

LGTM, I think changing this to support more architectures can happen once those are supported and shouldn't slow progress here.

This revision is now accepted and ready to land.Mar 23 2020, 9:18 PM
phosek accepted this revision.Mar 24 2020, 9:12 PM
phosek added inline comments.
libc/test/loader/CMakeLists.txt
41

Leftover?

63

This is likely going to fail on any non-POSIX systems since there likely won't be sh available, should we print a warning in that case?

abrachet added inline comments.Mar 24 2020, 9:31 PM
libc/test/loader/CMakeLists.txt
63

I just tried this with fish (the one I described as not accepting the syntax VAR=value) and it worked just fine. Removing sh -c, is fine then. I'm guessing though that you are saying the syntax VAR=value wont work in this case?

  • Remove a leftover if block in add_loader_test.
  • Add warning in the add_loader_test rule if the host is not unix.
sivachandra marked 3 inline comments as done.Mar 24 2020, 10:08 PM
sivachandra added inline comments.
libc/test/loader/CMakeLists.txt
63

I removed the sh -c now and added a warning at the beginning of this function.

This revision was automatically updated to reflect the committed changes.