Skip to content

[NFC][sanitizer] Switch to gnu_get_libc_version #108724

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Conversation

vitalybuka
Copy link
Collaborator

gnu_get_libc_version unlike confstr is not
intercepted. We should be able to use this
function earier.

Looks like we use confstr staring from
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60038
but there is no specific reason to refer it over
gnu_get_libc_version.

@llvmbot
Copy link
Member

llvmbot commented Sep 15, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Vitaly Buka (vitalybuka)

Changes

gnu_get_libc_version unlike confstr is not
intercepted. We should be able to use this
function earier.

Looks like we use confstr staring from
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=60038
but there is no specific reason to refer it over
gnu_get_libc_version.


Full diff: https://github.com/llvm/llvm-project/pull/108724.diff

1 Files Affected:

  • (modified) compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp (+8-10)
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
index 071ecc4516e0f0..bc3a41bba03fc1 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux_libcdep.cpp
@@ -40,6 +40,10 @@
 #  include <sys/resource.h>
 #  include <syslog.h>
 
+#  ifdef SANITIZER_GLIBC
+#    include <gnu/libc-version.h>
+#  endif
+
 #  if !defined(ElfW)
 #    define ElfW(type) Elf_##type
 #  endif
@@ -198,17 +202,11 @@ bool SetEnv(const char *name, const char *value) {
 
 __attribute__((unused)) static bool GetLibcVersion(int *major, int *minor,
                                                    int *patch) {
-#  ifdef _CS_GNU_LIBC_VERSION
-  char buf[64];
-  uptr len = confstr(_CS_GNU_LIBC_VERSION, buf, sizeof(buf));
-  if (len >= sizeof(buf))
-    return false;
-  buf[len] = 0;
-  static const char kGLibC[] = "glibc ";
-  if (internal_strncmp(buf, kGLibC, sizeof(kGLibC) - 1) != 0)
-    return false;
-  const char *p = buf + sizeof(kGLibC) - 1;
+#  ifdef SANITIZER_GLIBC
+  const char *p = gnu_get_libc_version();
   *major = internal_simple_strtoll(p, &p, 10);
+  // Caller do not expect anything else.
+  CHECK_EQ(*major, 2);
   *minor = (*p == '.') ? internal_simple_strtoll(p + 1, &p, 10) : 0;
   *patch = (*p == '.') ? internal_simple_strtoll(p + 1, &p, 10) : 0;
   return true;

@MaskRay
Copy link
Member

MaskRay commented Sep 16, 2024

The header gnu/libc-version.h has been in glibc available since 1998. Nice!

clementval and others added 2 commits September 16, 2024 09:50
Created using spr 1.3.4

[skip ci]
Created using spr 1.3.4
@vitalybuka vitalybuka changed the base branch from users/vitalybuka/spr/main.nfcsanitizer-switch-to-gnu_get_libc_version to main September 16, 2024 16:51
Created using spr 1.3.4
@vitalybuka vitalybuka merged commit 69f3244 into main Sep 16, 2024
5 of 6 checks passed
@vitalybuka vitalybuka deleted the users/vitalybuka/spr/nfcsanitizer-switch-to-gnu_get_libc_version branch September 16, 2024 16:53
thurstond added a commit that referenced this pull request Sep 16, 2024
This reverts commit 69f3244.

Reason: buildbot breakage because Android doesn't have <gnu/libc-version.h>
https://lab.llvm.org/buildbot/#/builders/186/builds/2381

(It's probably easy to fix but I don't readily have an Android device to test.)
vitalybuka added a commit that referenced this pull request Sep 16, 2024
…#108885)

In #108724 `#ifdef` was used instead of `#if`.

This reverts commit 68e4518.
tmsri pushed a commit to tmsri/llvm-project that referenced this pull request Sep 19, 2024
…)" (llvm#108885)

In llvm#108724 `#ifdef` was used instead of `#if`.

This reverts commit 68e4518.
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants