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

[Chandler Carruth]

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...


> This is worth doing only for the interceptors and not for the rest of the
> code.
>

Clearly. It's about matching interfaces not about these types being
superior in some way. =]


> --kcc
>
>
>>
>> Fails to compile on windows. If it does, I would suggest a more targeted
>> fix rather than avoiding stddef.h / size_t... Providing a tiny stub of code
>> to include stddef.h, and forcibly typedef size_t to something reasonable if
>> MSVC has left us hanging seems much better than avoiding size_t in standard
>> spec'ed interfaces.
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120625/c2d4d472/attachment.html>