[PATCH] D46638: [sanitizer] Fix runtime crash on 32-bit Linux with glibc 2.27

[Peter Wu via Phabricator via llvm-commits]

Lekensteyn added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:165
+  glibc_version_fn get_glibc_version =
+    (glibc_version_fn)dlsym(RTLD_NEXT, "gnu_get_libc_version");
+  if (!get_glibc_version)
----------------
jakubjelinek wrote:
> So you both call dlsym (instead of the function directly) and then parse the string?  That is significantly larger and more expensive than just calling the dlvsym.  Furthermore, other code in the sanitizers use confstr for this purpose.
Detecting glibc 2.27 by lookup up the glob symbol seems a bit indirect, that is why I preferred an approach that directly checks the version.

There is only one `confstr(_CS_GNU_LIBC_VERSION)` user (which got removed below). String parsing costs should be negligible and there are enough `dlsym` calls for interceptors such that this additional one should not matter.

I think that the few cycles extra here is worth the lower code maintenance cost that would otherwise be necessary for indirectly retrieving `strconf` without breaking msan. Correct me if I am wrong :)


================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:202
+  int minor, patch;
+  GetGlibcVersion(&minor, &patch);
+  // Glibc before 2.27 used a different calling convention for
----------------
jakubjelinek wrote:
> Ugh, so you call this unconditionally on all architectures, just because one needs it?
What about this?

    int minor = 0, patch = 0;
    if (CHECK_GET_TLS_STATIC_INFO_VERSION) GetGlibcVersion(&minor, &patch);
    if (CHECK_GET_TLS_STATIC_INFO && minor && minor < 27) {

Alternative: make GetGlibcVersion return `(minor << 8) | patch` or a large number if parsing failed for some reason. Then: `if (CHECK_GET_TLS_STATIC_INFO && GetGlibcVersion() < 27)`.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D46638