[llvm-commits] [compiler-rt] r159132 - /compiler-rt/trunk/lib/asan/asan_malloc_linux.cc

[Kostya Serebryany]

On Mon, Jun 25, 2012 at 3:48 PM, Chandler Carruth <chandlerc at google.com>wrote:

> On Mon, Jun 25, 2012 at 4:38 AM, Kostya Serebryany <kcc at google.com> wrote:
>
>>
>>
>> On Mon, Jun 25, 2012 at 2:41 PM, Chandler Carruth <chandlerc at google.com>wrote:
>>
>>> On Mon, Jun 25, 2012 at 3:37 AM, Kostya Serebryany <kcc at google.com>wrote:
>>>
>>>>
>>>>
>>>> On Mon, Jun 25, 2012 at 2:28 PM, Chandler Carruth <chandlerc at google.com
>>>> > wrote:
>>>>
>>>>> On Mon, Jun 25, 2012 at 3:18 AM, Kostya Serebryany <kcc at google.com>wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On Mon, Jun 25, 2012 at 2:15 PM, Chandler Carruth <
>>>>>> chandlerc at google.com> wrote:
>>>>>>
>>>>>>> On Mon, Jun 25, 2012 at 2:58 AM, Kostya Serebryany <kcc at google.com>wrote:
>>>>>>>
>>>>>>>> Author: kcc
>>>>>>>> Date: Mon Jun 25 04:58:29 2012
>>>>>>>> New Revision: 159132
>>>>>>>>
>>>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=159132&view=rev
>>>>>>>> Log:
>>>>>>>> [asan] get rid of '#include <malloc.h>' in the implementation of
>>>>>>>> malloc interceptors
>>>>>>>>
>>>>>>>> Modified:
>>>>>>>>    compiler-rt/trunk/lib/asan/asan_malloc_linux.cc
>>>>>>>>
>>>>>>>> Modified: compiler-rt/trunk/lib/asan/asan_malloc_linux.cc
>>>>>>>> URL:
>>>>>>>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/asan/asan_malloc_linux.cc?rev=159132&r1=159131&r2=159132&view=diff
>>>>>>>>
>>>>>>>> ==============================================================================
>>>>>>>> --- compiler-rt/trunk/lib/asan/asan_malloc_linux.cc (original)
>>>>>>>> +++ compiler-rt/trunk/lib/asan/asan_malloc_linux.cc Mon Jun 25
>>>>>>>> 04:58:29 2012
>>>>>>>> @@ -20,15 +20,13 @@
>>>>>>>>  #include "asan_internal.h"
>>>>>>>>  #include "asan_stack.h"
>>>>>>>>
>>>>>>>> -#include <malloc.h>
>>>>>>>> -
>>>>>>>>  #ifdef ANDROID
>>>>>>>>  struct MallocDebug {
>>>>>>>> -  void* (*malloc)(size_t bytes);
>>>>>>>> +  void* (*malloc)(uptr bytes);
>>>>>>>>
>>>>>>>
>>>>>>> This seems really wrong to me... There are definitely platforms
>>>>>>> where sizeof(size_t) != sizeof(void*)...
>>>>>>>
>>>>>>
>>>>>> There are such platforms indeed, but asan will be broken on such
>>>>>> systems in all other possible ways, not just this.
>>>>>>
>>>>>
>>>>> Why not define these according to the standard?
>>>>>
>>>>> In particular, I don't understand an aversion to 'stddef.h' and
>>>>> 'size_t'. These don't come from the OS, they come from the standard and
>>>>> from the compiler. The 'stddef.h' that ships with Clang doesn't include any
>>>>> other headers. It cannot be simulated by your code because it leverages
>>>>> implementation-details to expose standard interfaces such as 'size_t'.
>>>>>
>>>>> For example, 'stddef.h' can be included into code that is compiled
>>>>> with -ffreestanding, and thus without any system libraries or headers. It
>>>>> seems like that should be a sufficiently strict bar for portability for
>>>>> ASan?
>>>>>
>>>>>
>>>>>> Otoh, this change protects us from problems like this one, where
>>>>>> malloc.h has some unexpected features.
>>>>>>
>>>>>
>>>>> My comment here isn't about including 'malloc.h', but about using
>>>>> 'uptr' instead of 'size_t'.
>>>>>
>>>>
>>>> It is indeed very pitty that we can not use the standard size_t. I'd
>>>> love to be able to.
>>>> This has something to do with Windows.
>>>> We can not build asan run-time with clang on Windows (at least yet), so
>>>> we have to build it with MSVC.
>>>> There, stddef.h is not nice at all. (Frankly, I don't remember exactly,
>>>> but I think it just doesn't define size_t at all).
>>>>
>>>
>>> Wait, what?
>>>
>>> I can't really believe this:
>>>
>>> #include <stddef.h>
>>>
>>
>>> size_t x = 42u;
>>>
>>
>> It's actually the other way around: stddef.h on windows defines way too
>> much.
>> The pre-processed output for this test is 33K!
>> So, if we include stddef.h on windows, we end up with tons of stuff that
>> conflicts more or less with everything we do in asan.
>>
>> We could define our own __sanitizer::size_t, use it for all interceptors
>> that have size_t in the interface,
>> and in a separate file (where we can include stddef.h) make sure that
>> sizeof(::size_t)==sizeof(__sanitizer::size_t).
>> WDYT?
>>
>
> Yuck. ;]
>
> How about a slightly different approach, in sanitizer_stddef.h or some
> other common header:
>
> #ifndef _WIN32
> # include <stddef.h>
> #else
>
> // Provide custom typedefs emulating the minimal interface that
> // should be exported by stddef.h -- The header on
> // Windows is too bloated to use.
> typedef uptr size_t;
> typedef sptr ptrdiff_t;
> ...
>
> #endif // _WIN32
>
>
> My thought is that you really only need a few things from stddef, and you
> can get them directly when reasonable, and provide them yourself when the
> MSVC header gets in the way...
>
>

We actually had something similar.
But then, there are Linux, Mac and Android which we support now; more
systems may be added in future.
Who knows what else is lurking in someone's stddef.h!?
E.g. is NULL defined there and shell we use it then (currently, we don't)?
Once a system header is included into every file in asan it will be very
hard to get rid of it, so I prefer to not even try.

--kcc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120625/10788371/attachment.html>