Page MenuHomePhabricator

[asan] Enable asan Win64 build
ClosedPublic

Authored by wang0109 on May 19 2016, 4:12 PM.

Details

Summary

This patch is activating the build of Asan on Windows 64-bits.
It's fixing compilation errors. The runtime is not yet working.

Missing features:

  • OverrideFunction for x64
  • an equiv function for inline asm (atomic_compare_exchange_strong)
  • shadow memory offset needs to be adjusted
  • RoundUpToInstrBoundary for x64

They will be implemented by subsequent patches.

Diff Detail

Repository
rL LLVM

Event Timeline

wang0109 updated this revision to Diff 57876.May 19 2016, 4:12 PM
wang0109 retitled this revision from to [asan] Enable asan Win64 build.
wang0109 updated this object.
wang0109 added a reviewer: rnk.
rnk added inline comments.May 19 2016, 4:25 PM
cmake/config-ix.cmake
438 ↗(On Diff #57876)

I think this is likely to turn this bot red:
http://lab.llvm.org:8011/builders/clang-x64-ninja-win7/

It builds 'check-all' with compiler-rt on win64, and the tests obviously don't pass yet. I think if you change the code at the end of compiler-rt/test/asan/CMakeLists.txt to set EXCLUDE_FROM_ALL to ON, you can avoid breaking it.

lib/interception/interception_win.cc
154 ↗(On Diff #57876)

Space after //, here and elsewhere

etienneb updated this object.May 19 2016, 7:41 PM
wang0109 updated this revision to Diff 57932.May 20 2016, 7:47 AM

Update the patch to clean up the spaces and add more comment

wang0109 updated this revision to Diff 57961.May 20 2016, 11:27 AM
  • Update diff for cmake - avoid buildbot fail (need more tests)
rnk added inline comments.May 23 2016, 11:05 AM
test/asan/CMakeLists.txt
96 ↗(On Diff #57961)

You actually want to do this here to affect the main asan test suite, not just the dynamic asan test suite:

if (OS_NAME MATCHES "Windows" AND CMAKE_SIZEOF_VOID_P EQUAL 8)
  set(EXCLUDE_FROM_ALL TRUE)
endif()
wang0109 updated this revision to Diff 58331.May 24 2016, 3:21 PM
  • Update cmake files to disable unsupported tests including asan and ubsan with asan
rnk edited edge metadata.May 24 2016, 3:29 PM

Functionally this is fine. Etienne, can you commit this on behalf of Wei after the comment stuff is addressed?

lib/sanitizer_common/sanitizer_atomic_msvc.h
179 ↗(On Diff #58331)

Can you capitalize your comment sentence starts and end them with a period? Sorry I forgot to raise that earlier.

wang0109 updated this revision to Diff 58413.May 25 2016, 7:20 AM
wang0109 edited edge metadata.
  • Update comment format
wang0109 updated this revision to Diff 58414.May 25 2016, 7:22 AM
  • Update comment format again
etienneb edited edge metadata.May 25 2016, 7:32 AM
In D20455#438465, @rnk wrote:

Functionally this is fine. Etienne, can you commit this on behalf of Wei after the comment stuff is addressed?

Ok. I'll do.
I'll try the patch too.

wang0109 updated this revision to Diff 58435.May 25 2016, 9:03 AM
wang0109 edited edge metadata.
  • Update diff for linting style

I tested the patch on my windows machine, and it's working fine.
I'm running unittests on linux.

Could you please fix the remaining nits and I'll land this.

lib/asan/asan_win.cc
73 ↗(On Diff #58435)

no @

156 ↗(On Diff #58435)

There is no "@" here.

lib/interception/interception_win.cc
72 ↗(On Diff #58435)

no @

154 ↗(On Diff #58435)

no @

test/asan/CMakeLists.txt
6 ↗(On Diff #58435)

missing (wwchrome)

126 ↗(On Diff #58435)

ditto

test/ubsan/CMakeLists.txt
35 ↗(On Diff #58435)

ditto

wang0109 updated this revision to Diff 58793.May 27 2016, 8:44 AM
  • Update diff to fix username in TODO

still one missing

lib/sanitizer_common/sanitizer_atomic_msvc.h
179 ↗(On Diff #58793)

no @

wang0109 updated this revision to Diff 58797.May 27 2016, 8:53 AM
  • Fix username in TODO again.

lgtm

I'll land this soon.

etienneb accepted this revision.May 27 2016, 9:00 AM
etienneb edited edge metadata.

lg

This revision is now accepted and ready to land.May 27 2016, 9:00 AM
Closed by commit rL271049 (authored by etienneb). · Explain WhyMay 27 2016, 2:36 PM
This revision was automatically updated to reflect the committed changes.
kubamracek added inline comments.Jun 5 2016, 8:18 AM
compiler-rt/trunk/test/asan/CMakeLists.txt
6–10

I’m not getting this change. I read it like 20 times now, but it still seems it’s disabling ASan tests everywhere, *except* for Win64.