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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_linux_libcdep.cc:160
 
+#if !SANITIZER_ANDROID
+void GetGlibcVersion(int *minor, int *patch) {
----------------
I'd split this into two patches:
1. Extract code into GetGlibcVersion
2. GetTls changes


================
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
----------------
Lekensteyn wrote:
> 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)`.
I am not sure that we need these 
CHECK_EQ(sizeof(get_tls), sizeof(get_tls_static_info_ptr))
and 
internal_memcpy
If not we can go with:

'''
static bool IsGetTlsInternal() {
if (!CHECK_GET_TLS_STATIC_INFO_VERSION)
  return false;
 int minor;
 GetGlibcVersion(&minor, &patch);
 return minor && minor < 27;
}

static void GetTlsSize(size_t *tls_size, size_t *tls_align) {
  void *get_tls_static_info_ptr = dlsym(RTLD_NEXT, "_dl_get_tls_static_info");
  CHECK_NE(get_tls_static_info_ptr, 0);
  if (IsGetTlsInternal()) {
    typedef void (*get_tls_func)(size_t *, size_t *);
    ((get_tls_func)get_tls_static_info_ptr)(tls_size, tls_align);
  } else {
    typedef void (*get_tls_func)(size_t *, size_t *) DL_INTERNAL_FUNCTION;
    ((get_tls_func)get_tls_static_info_ptr)(tls_size, tls_align);
  }
}
'''


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D46638