This is an archive of the discontinued LLVM Phabricator instance.

[asan] Use 32TB as shadow offset for asan on Windows64
ClosedPublic

Authored by wang0109 on Jun 20 2016, 11:34 AM.

Details

Summary

This is part of the effort for asan to support Windows 64 bit.
The large offset is being tested on Windows 10 (which has larger usable
virtual address space than Windows 8 or earlier)

Diff Detail

Event Timeline

wang0109 updated this revision to Diff 61273.Jun 20 2016, 11:34 AM
wang0109 retitled this revision from to [asan] Use 32TB as shadow offset for asan on Windows64.
wang0109 updated this object.
wang0109 added reviewers: etienneb, rnk, chrisha.
wang0109 updated this revision to Diff 61274.Jun 20 2016, 11:37 AM
  • update diff: add ULL to end of offsets
wang0109 updated this revision to Diff 61277.Jun 20 2016, 11:39 AM
  • update diff: use shift notation
filcab added a subscriber: filcab.Jun 20 2016, 11:48 AM
filcab added inline comments.
lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
755

Don't change this. This code is suspicious and seems to have either bit-rotted or still only support Linux.

lib/Transforms/Instrumentation/AddressSanitizer.cpp
82

Since all the other 1-bit-set constants around here use 1ULL << XX, can you switch to 1ULL << 45?

etienneb added inline comments.Jun 20 2016, 11:55 AM
lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
755

Agree, this will break linux instrumentation.

Note: Google Test filter = AddressSanitizer.asm_rw
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from AddressSanitizer
[ RUN      ] AddressSanitizer.asm_rw
/usr/local/google/home/etienneb/llvm/llvm/projects/compiler-rt/lib/asan/tests/asan_asm_test.cc:135: Failure
Death test: asm_write(&buf[1], static_cast<T>(0))
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/usr/local/google/home/etienneb/llvm/llvm/projects/compiler-rt/lib/asan/tests/asan_asm_test.cc:135: Failure
Death test: asm_write(&buf[1], static_cast<T>(0))
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/usr/local/google/home/etienneb/llvm/llvm/projects/compiler-rt/lib/asan/tests/asan_asm_test.cc:135: Failure
Death test: asm_write(&buf[1], static_cast<T>(0))
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/usr/local/google/home/etienneb/llvm/llvm/projects/compiler-rt/lib/asan/tests/asan_asm_test.cc:135: Failure
Death test: asm_write(&buf[1], static_cast<T>(0))
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/usr/local/google/home/etienneb/llvm/llvm/projects/compiler-rt/lib/asan/tests/asan_asm_test.cc:149: Failure
Death test: asm_write<__m128i>((__m128i*) p, val)
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/usr/local/google/home/etienneb/llvm/llvm/projects/compiler-rt/lib/asan/tests/asan_asm_test.cc:158: Failure
Death test: asm_read(&buf[1])
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/usr/local/google/home/etienneb/llvm/llvm/projects/compiler-rt/lib/asan/tests/asan_asm_test.cc:158: Failure
Death test: asm_read(&buf[1])
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/usr/local/google/home/etienneb/llvm/llvm/projects/compiler-rt/lib/asan/tests/asan_asm_test.cc:158: Failure
Death test: asm_read(&buf[1])
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/usr/local/google/home/etienneb/llvm/llvm/projects/compiler-rt/lib/asan/tests/asan_asm_test.cc:158: Failure
Death test: asm_read(&buf[1])
    Result: failed to die.
 Error msg:
[  DEATH   ] 
/usr/local/google/home/etienneb/llvm/llvm/projects/compiler-rt/lib/asan/tests/asan_asm_test.cc:170: Failure
Death test: asm_read<__m128i>((__m128i*) p)
    Result: failed to die.
 Error msg:
[  DEATH   ] 
[  FAILED  ] AddressSanitizer.asm_rw (165 ms)
[----------] 1 test from AddressSanitizer (165 ms total)
wang0109 updated this revision to Diff 61292.Jun 20 2016, 12:40 PM
  • update diff: guard for windows 64, no impact on linux
etienneb added inline comments.Jun 20 2016, 1:00 PM
lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
754

It's still not valid.
Clang can cross compile, you it must be able to emit any of these two constants, depending on the target.

filcab added inline comments.Jun 20 2016, 1:50 PM
lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
754

This class doesn't get instantiated if the target is not Linux on x86-64. This is also not the way to make it support Windows. You can't use the pre-processor to support it, since we might be cross-compiling.
Please don't touch this file yet. Deal with plain ASan first, and afterwards enhance it by enabling ASM instrumentation.

X86AsmInstrumentation *
CreateX86AsmInstrumentation(const MCTargetOptions &MCOptions,
                            const MCContext &Ctx, const MCSubtargetInfo *&STI) {
  Triple T(STI->getTargetTriple());
  const bool hasCompilerRTSupport = T.isOSLinux();
  if (ClAsanInstrumentAssembly && hasCompilerRTSupport &&
      MCOptions.SanitizeAddress) {
    if (STI->getFeatureBits()[X86::Mode32Bit] != 0)
      return new X86AddressSanitizer32(STI);
    if (STI->getFeatureBits()[X86::Mode64Bit] != 0)
      return new X86AddressSanitizer64(STI);
  }
  return new X86AsmInstrumentation(STI);
}

Looks like, instrumentation of inline asm is only for Linux. On windows, it is not used. Also, windows 64's Microsoft compiler has no support for inline asm anyway.

Yes, I am reverting the change to X86AsmInstrumentation.

wang0109 updated this revision to Diff 61303.Jun 20 2016, 2:00 PM
  • update diff: Revert change to 64bit asm instrumentation offset
rnk accepted this revision.Jun 21 2016, 7:27 AM
rnk edited edge metadata.

lgtm

We could have a test in llvm/test/Instrumentation/AddressSanitizer/win64.ll like the freebsd.ll test, but it seems fairly trivial and low value.

lib/Transforms/Instrumentation/AddressSanitizer.cpp
403–404

clang-format would put the 'else' on the line with the previous '}'.

This revision is now accepted and ready to land.Jun 21 2016, 7:27 AM
wang0109 updated this revision to Diff 61369.Jun 21 2016, 7:39 AM
wang0109 edited edge metadata.
  • update diff: fix formatting
This revision was automatically updated to reflect the committed changes.