[libc-commits] [PATCH] D79192: [libc] Add integration tests.

[Paula Toth via Phabricator via libc-commits]

PaulkaToast marked 2 inline comments as done.
PaulkaToast added a comment.

In D79192#2015295 <https://reviews.llvm.org/D79192#2015295>, @sivachandra wrote:

> - Instead of creating a target for every entrypoint, have you considered generating a single file which:
>   1. Includes all header files listed in the `api.td`.
>   2. Lists `cpp:Function` pointers to all functions as required by `api.td`.
>
>     There definitely are problems that need to be solved with this approach. For example, how do you ensure all public header files are already present in the `build/.../libc/include` directory. But, the appeal of this approach is that one does not have to create explicit or implicit targets for "integration tests". There would be just one target which would test everything in one go.


Good idea, this is done and the new integration test file now looks like this:

  ...
  #include <unistd.h>
  #include <sys/mman.h>
  #include <math.h>
  
  int main() {
    static_assert(__llvm_libc::cpp::IsSame<int(int), decltype(raise)>::Value, "raise prototype in TableGen does not match public header");
    static_assert(__llvm_libc::cpp::IsSame<int(int, const struct sigaction *__restrict, struct sigaction *__restrict), decltype(sigaction)>::Value, "sigaction prototype in TableGen does not match public header");
    static_assert(__llvm_libc::cpp::IsSame<int(sigset_t *, int), decltype(sigdelset)>::Value, "sigdelset prototype in TableGen does not match public header");
  ...

Thanks to @abrachet's suggestion of using `IsSame` we also get nice error messages. As for ensuring all public header files are present I added a top-level target for the headers that we can depend on here (same as @abrachet mentioned)

> - In general, I don't think we should expect that internal API to match the public API. For example, we might choose to give internal types names which are different from the public names. So, I think the second kind of tests you mention in your description is not required as it is restrictive.

Done, this only tests the public/libc user perspective now.

> - Using `-nostdinc` would require us to explicitly list path to the linux headers for entrypoints which depend on the linux header files, which will very likely bring all headers into the include path. Do you have any ideas on how to get around it?

For now I've commented out `-nostdinc` with a TODO, the most straightforward way would be to have our public headers not depend on Linux dev headers but another approach could be to use our clang-tidy `RestrictSystemIncludes` check to ensure only the right Linux header gets included (this would add a dependency on `clang-tidy` for the integration tests though)

> - Using `-nostdlib` results in an incomplete link. For ELF, most linkers only give a warning about missing `_start` symbol, but the resulting exe is bad/incomplete. Do you have any plans to ensure that the link is complete, resulting in an exe which can be executed?

My earlier iteration of this had us linking with the loader for a complete executable link. However, since we now check everything statically, I think it's enough if the integration test compiles and links as this ensures the libc symbols are present and the types are compatible. We could add the loader again if you think it's necessary.



================
Comment at: libc/utils/HdrGen/PrototypeTestGen/PrototypeTestGen.cpp:48
+  OS << "int main() {\n";
+  OS << "  __llvm_libc::cpp::function<";
+  auto G = makeAPIReader(PublicHeaderFileOption, records);
----------------
abrachet wrote:
> It was just recently renamed to `Function` But I think it makes more semantic sense to use `cpp::IsSame<[type], decltype(EntryPointNameOption)>`
Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79192/new/

https://reviews.llvm.org/D79192