[libc-commits] [PATCH] D130966: [libc] Add init and fini array iteration to the loader.

[Alex Brachet via Phabricator via libc-commits]

abrachet added a comment.

Got these comments in a little late, oops



================
Comment at: libc/loader/linux/x86_64/start.cpp:93
 
+using InitCallback = void(int, char **, char **);
+using FiniCallback = void(void);
----------------
It's not clear to me why these take argc, argv and env. .init_array functions wouldn't take these because in the general they would only be meaningful when loading an executable and not a shared object. I suppose it isn't harmful to pass arguments to a function which doesn't take them, though it seems surprising given likely no constructor functions in the wild take these.


================
Comment at: libc/loader/linux/x86_64/start.cpp:118
+  size_t fini_array_size = __fini_array_end - __fini_array_start;
+  for (size_t i = 0; i < fini_array_size; ++i)
+    reinterpret_cast<FiniCallback *>(__fini_array_start[i])();
----------------
These should happen in reverse order.


================
Comment at: libc/test/integration/loader/linux/cxx_globals_test.cpp:17
+  A(int a) : val(a) {}
+  // TODO: when we have implementation for __cxa_atexit, an explicit definition
+  // of the destructor should be provided to test that path of registering the
----------------
sivachandra wrote:
> abrachet wrote:
> > `__cxa_atexit` is a alternative for the `.fini_array`, it doesn't use it
> I am not sure I understand this comment. My intention here was to say we cannot yet have a non-trivial destructor as it will lead to a call to `__cxa_atexit` which we do not yet have.
I see sorry, I thought it was specific to .fini_array


================
Comment at: libc/test/integration/loader/linux/init_fini_array_test.cpp:39
+}
+__attribute__((destructor)) void reset_initval() { initval = 0; }
+
----------------
If the integration tests check that the process exited normally you could return 1 from main and _Exit(0) here to ensure this is called. Additionally it might make sense to specify a priority to destructor and constructor, like destructor(num) to ensure they are called in the right order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130966