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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/loader/linux/x86_64/start.cpp:93
 
+using InitCallback = void(int, char **, char **);
+using FiniCallback = void(void);
----------------
abrachet wrote:
> 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.
Neither do I know of any practical examples. Most libcs do not pass any args to the init functions. However, this patch matches glibc behavior.

```
$ cat init.cpp
#include <stdio.h>

__attribute__((constructor)) void initfunc(int argc, char **argv, char **envp) {
  printf("argc: %d\n", argc);
  for (int i = 0; i < argc; ++i) {
    printf("\targ[%d] = %s\n", i, argv[i]);
  }
}

int main() {
  return 0;
}
$ clang++ init.cpp
$ ./a.out
argc: 1
	arg[0] = ./a.out
$ ./a.out abc def ghi
argc: 4
	arg[0] = ./a.out
	arg[1] = abc
	arg[2] = def
	arg[3] = ghi
```


================
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])();
----------------
abrachet wrote:
> These should happen in reverse order.
Oops, yes! I will fix it in the next pass. I am working on the `__cxa_atexit` hook up anyway so will fix it in that pass.


================
Comment at: libc/test/integration/loader/linux/init_fini_array_test.cpp:39
+}
+__attribute__((destructor)) void reset_initval() { initval = 0; }
+
----------------
abrachet wrote:
> 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.
OK - I will add more tests with destructor priority which I think would be better than what we have. Priorities + the `_Exit(0)` idea together will make it a complete test I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130966