[PATCH] UBSan on Windows

[Aaron Ballman]

On Fri, Oct 24, 2014 at 1:42 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
> 2014-10-24 10:38 GMT-07:00 Aaron Ballman <aaron at aaronballman.com>:
>> On Fri, Oct 24, 2014 at 1:32 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>> 2014-10-24 10:26 GMT-07:00 Aaron Ballman <aaron at aaronballman.com>:
>>>> On Fri, Oct 24, 2014 at 1:14 PM, Timur Iskhodzhanov <timurrrr at google.com> wrote:
>>>>> Hi Aaron,
>>>>>
>>>>> See also http://reviews.llvm.org/D5368 – Amine has made some progress
>>>>> on UBSan lately as well, but didn't quite finish it.
>>>>>
>>>>> Can you please send the review via phab?
>>>>
>>>> I prefer to avoid phab; it's never worked as part of my workflow. Is
>>>> it required for compiler-rt patches?
>>>
>>> Not required, we just prefer it this way :)
>>
>> I may give it another shot then, as time progresses. :-)
>>
>>>
>>>>> Comments:
>>>>> * Your implementation of atomic_exchange doesn't seem to be atomic?
>>>>
>>>> The same is true of all the atomic_exchange functions, no? This one is
>>>> simply a copy of the other two, with the registers corrected for a
>>>> 32-bit value.
>>>
>>> Oh that's surprising the other implementations were done this way.
>>> [I didn't see it 'cause there wasn't enough context in the patch]
>>
>> Ah!
>>
>>>
>>>> We should probably be using the InterlockedExchange functions on
>>>> Windows instead of hand-rolling our own, but I felt that was a
>>>> separate change for another patch.
>>>
>>> Sounds reasonable, but please add a FIXME then.
>>
>> Added.
>>
>>> Dmitry, can you please take a look?
>>>
>>>>> * lib/ubsan/ubsan_value.h – please update the comment
>>>>
>>>> Can do.
>>>>
>>>>> * Does check-ubsan (?) work?
>>>>
>>>> I cannot get any of the check-*san tests to work within MSVC. There
>>>> are significant CMake issues for this workflow, which I'm hoping to
>>>> address as I have the time. For instance, I get a considerable number
>>>> of link errors because the libraries are placed in:
>>>>
>>>> E:\llvm\2013\Release\lib\clang\3.6.0\lib\windows\Release\
>>>>
>>>> But the linker is given a path to find them at:
>>>>
>>>> E:\llvm\2013\Release\lib\clang\3.6.0\lib\windows\
>
> btw, this is http://llvm.org/bugs/show_bug.cgi?id=20885
>
>>>> When I attempt to run check-ubsan, I get:
>>>>
>>>> 59>  Building Custom Rule
>>>> E:/llvm/llvm/projects/compiler-rt/test/ubsan/CMakeLists.txt
>>>> 59>  CMake does not need to re-run because
>>>> E:\llvm\2013\projects\compiler-rt\test\ubsan\CMakeFiles\generate.stamp
>>>> is up-to-date.
>>>> 59>  Running UndefinedBehaviorSanitizer tests
>>>> 59>  lit.py: LitConfig.py:79: warning: Unable to find 'bash'.
>>>> 59>  lit.py: lit.common.cfg:20: fatal: Can't find compiler on path
>>>> 'E:/llvm/2013/$(Configuration)/bin/clang.exe'
>>>> 59>C:\Program Files
>>>> (x86)\MSBuild\Microsoft.Cpp\v4.0\V120\Microsoft.CppCommon.targets(170,5):
>>>> error MSB6006: "cmd.exe" exited with code 2.
>>>>
>>>> I get this same error when attempting to run check-asan after manually
>>>> fixing up the library paths.
>>>
>>> Can you try to use ninja-based build for the time being?
>>
>> I can, but have a strong preference for not using that. It certainly
>> shouldn't be required (which is why I've spent a bunch of time
>> recently trying to fix the CMake instead).
>
> Sure, but it's better to fix one thing at a time rather than tie two
> problems together :)

Strongly agreed!

~Aaron
>
>>>>> * Have you tried Win64?
>>>>
>>>> I have not, just x86.
>>>
>>> Please try and/or see Amine's patch – he has only enabled UBSan on 32-bit arch.
>>
>> Corrected in the attached patch, along with some other nits.
>
> I'll give it a try today.
>
>> Thanks!
>>
>> ~Aaron