[PATCH] D44623: Fix asan on i?86-linux (32-bit) against glibc 2.27 and later

[Peter Wu via Phabricator via llvm-commits]

Lekensteyn added a comment.

@vitalybuka I think that this patch is making good progress, so I abandoned the other patch.

If forward compatibility is important, I would change the `__GLIBC_PREREQ` check. Otherwise it looks ready to me.



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cc:174
+
+#if CHECK_GET_TLS_STATIC_INFO_VERSION
 # define DL_INTERNAL_FUNCTION __attribute__((regparm(3), stdcall))
----------------
vitalybuka wrote:
> jakubjelinek wrote:
> > vitalybuka wrote:
> > > Do we need this if we do runtime check anyway?
> > > We can't get into GetTlsStaticInfoRegparmCall with CHECK_GET_TLS_STATIC_INFO_VERSION == 0
> > If you mean the __GLIBC_PREREQ, that is an optimization to avoid doing the runtime check if we know the check is unnecessary.
> > If the CallGetTls<GetTlsStaticInfoCall>(get_tls_static_info_ptr, ...); call is not ifdefed out through preprocessor, then yes, it is needed.
> > 
> This initialization once per process not thread, so I don't see reasons to complicate code for such minor improvement.
For forward compatibility, what about dropping `__GLIBC_PREREQ`? E.g.:
```
#if defined(__i386__) && defined(__GLIBC_PREREQ)
# define CHECK_GET_TLS_STATIC_INFO_VERSION 1
# define DL_INTERNAL_FUNCTIOn ...
#else
# define CHECK_GET_TLS_STATIC_INFO_VERSION 0
#endif
```

Then a binary built with clang on glibc 2.26 will still work correctly with glibc 2.27.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cc:238
   uptr len = confstr(_CS_GNU_LIBC_VERSION, buf, sizeof(buf));
   if (len < sizeof(buf) && internal_strncmp(buf, "glibc 2.", 8) == 0) {
     char *end;
----------------
vitalybuka wrote:
> jakubjelinek wrote:
> > vitalybuka wrote:
> > > Is this the same as __GLIBC_PREREQ()
> > confstr is a runtime check, __GLIBC_PREREQ is a compile time check.
> Right,
> than this is should be similar to dlvsym(RTLD_NEXT, "glob", "GLIBC_2.27"))?
It is not possible, there are multiple versions being checked. D46638 tried to reuse this version check with the above, but that failed because `confstr` is intercepted by MSAN.

The better alternative seems `gnu_get_libc_version`, but there were concerns about `dlsym` being expensive. Not that big of a deal imo, but as the current patch looks fine to me, I dropped the other patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D44623