This is an archive of the discontinued LLVM Phabricator instance.

[sanitizers] Restore internal_readlink for x32
ClosedPublic

Authored by hjl.tools on Feb 19 2019, 3:38 PM.

Details

Summary

r316591 has

@@ -389,13 +383,11 @@ uptr internal_dup2(int oldfd, int newfd) {
 }

 uptr internal_readlink(const char *path, char *buf, uptr bufsize) {
-#if SANITIZER_NETBSD
-  return internal_syscall_ptr(SYSCALL(readlink), path, buf, bufsize);
-#elif SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
+#if SANITIZER_USES_CANONICAL_LINUX_SYSCALLS
   return internal_syscall(SYSCALL(readlinkat), AT_FDCWD,
                           (uptr)path, (uptr)buf, bufsize);
 #else
-  return internal_syscall(SYSCALL(readlink), (uptr)path, (uptr)buf, bufsize);
+  return internal_syscall_ptr(SYSCALL(readlink), path, buf, bufsize);
 #endif
 }

which dropped the (uptr) cast and broke x32. This patch puts back the
(uptr) cast to restore x32 and fixes:

https://bugs.llvm.org/show_bug.cgi?id=40783

Diff Detail

Repository
rL LLVM

Event Timeline

hjl.tools created this revision.Feb 19 2019, 3:38 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 19 2019, 3:38 PM
Herald added subscribers: Restricted Project, kubamracek. · View Herald Transcript
vitalybuka added inline comments.
lib/sanitizer_common/sanitizer_linux.cc
403 ↗(On Diff #187462)

Do we have a test which fails because of this?

Thanks,
I see, this very indirect test. Could you please add something into compiler-rt/lib/sanitizer_common/tests/sanitizer_common_test.cc
Maybe test ReadBinaryNameCached() which uses this function

hjl.tools updated this revision to Diff 187481.Feb 19 2019, 5:58 PM
hjl.tools edited the summary of this revision. (Show Details)
hjl.tools added a reviewer: dvyukov.
This revision is now accepted and ready to land.Feb 19 2019, 6:05 PM
This revision was automatically updated to reflect the committed changes.
morehouse added inline comments.
compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_common_test.cc
444

This test is failing on the Windows sanitizer bot: http://lab.llvm.org:8011/builders/sanitizer-windows/builds/42268

Please take a look.

morehouse added inline comments.Feb 21 2019, 9:08 AM
compiler-rt/trunk/lib/sanitizer_common/tests/sanitizer_common_test.cc
444

It looks like that ReadBinaryNameCached doesn't work on Windows.
Someone who is familiar with Windows should take a look.

morehouse added a subscriber: rnk.

@rnk: Could you help out here? I am clueless with Windows.

rnk added a comment.Feb 21 2019, 11:32 AM

@rnk: Could you help out here? I am clueless with Windows.

The answer is very straightforward, this code in sanitizer_win.cc:

uptr ReadBinaryName(/*out*/char *buf, uptr buf_len) {
  // FIXME: Actually implement this function.
  CHECK_GT(buf_len, 0);
  buf[0] = 0;
  return 0;
}

Of course the test fails, this always returns 0. Recommitting with the test disabled on Windows seems reasonable.

In D58413#1406201, @rnk wrote:

Of course the test fails, this always returns 0. Recommitting with the test disabled on Windows seems reasonable.

Could someone do it for me and test it on Windows? Thanks in advance.