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

[Chandler Carruth]

On Mon, Jun 25, 2012 at 5:01 AM, Kostya Serebryany <kcc at google.com> wrote:

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

While I'm generally sympathetic to this, I think we should distinguish
between *compiler* provided headers, and *system* provided headers...

I think the best way to achieve this distinction is to leverage the formal
requirements from C for a freestanding implementation. This mode only
provides a limited set af headers which often expose language / standard
interfaces which are not achievable directly through the language. IE, they
encapsulate compiler implementation details. These include 'stdarg.h' and
'stddef.h'; they are expected to be usable without linking *any* object
code into the binary, so they shouldn't be problematic for a runtime
library.

One way to ensure that you're abiding by these rules is to actually build
the runtime using '-ffreestanding', which should constrain you to a much
more limited set of headers and logic within those headers. For example,
'stdint.h' in C99 freestanding mode will *only* expose those types spelled
out in the C standard. Without the '-ffreestanding', you get all the POSIX
types, tons of other stuff you don't want.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120625/6354e541/attachment.html>