libsanitizer merge from upstream r175042

On Wed, Feb 13, 2013 at 05:39:15PM +0400, Konstantin Serebryany wrote:
> > No.  You can disable it for the whole system (prelink -ua), but that is not
> > a sane requirement to running sanitized programs.
> 
> Why not?
> :)

Because that is a fully system operation, requires root access, etc.
The fact that some user wants to test one of his programs with Asan
shouldn't need to affect other users.

> This we can deal with.
> We already setenv+reexec on Mac to solve similar issue with Mac's
> dynamic run-time.

The reexec is problematic, what if the program already in constructors run
before __asan_init (perhaps ctors of other libraries etc.) does something
that really shouldn't be done twice?

> > Sure, but it will be then slower, I thought you are looking for ASAN speed
> > improvements.
> 
> Yes, and we already achieved it on ubuntu :)

AFAIK prelink is available even on ubuntu, perhaps not the default.

> > I'll try to implement it eventually and
> > try to convince you ;)
> 
> That's surely not hard to implement, but very hard to support.

Why?

Here is the patch, works just fine for me here during asan.exp testing.
You can very easily either install and enable prelink on one of your
x86_64-linux testing boxes, or just install it and add test that
will say prelink -r 0x3600000000 some test shared library and then
just use it in sanitized program (that will also verify that you can mmap
libraries in that range), or even just write a test that will in a
non-instrumented ctor with lower priority than asan's priority
mmap a few pages at 0x3000000000 and close to 0x3fffff0000
and store some data into those buffers later on in sanitized code.



	Jakub

On Wed, Feb 13, 2013 at 04:19:14PM +0100, Jakub Jelinek wrote:
> 
> The reexec is problematic, what if the program already in constructors run
> before __asan_init (perhaps ctors of other libraries etc.) does something
> that really shouldn't be done twice?
> 

Jakub,
   Wouldn't sorting all of the constructors and destructors by priority, while
retaining the original order, in collect2 solve this issue (as was proposed for
implementing intra-modular init priority support on darwin)?
            Jack

On Wed, Feb 13, 2013 at 11:48:32AM -0500, Jack Howarth wrote:
> On Wed, Feb 13, 2013 at 04:19:14PM +0100, Jakub Jelinek wrote:
> > 
> > The reexec is problematic, what if the program already in constructors run
> > before __asan_init (perhaps ctors of other libraries etc.) does something
> > that really shouldn't be done twice?
> > 
> 
>    Wouldn't sorting all of the constructors and destructors by priority, while
> retaining the original order, in collect2 solve this issue (as was proposed for
> implementing intra-modular init priority support on darwin)?

I wasn't talking about darwin at all, and on Linux inter-CU init priority
works just fine.  I'm just saying that reexec isn't always safe, because
what the program does before __asan_init can be something undesirable to be
done multiple times.

	Jakub

On Wed, Feb 13, 2013 at 04:19:14PM +0100, Jakub Jelinek wrote:
> Here is the patch, works just fine for me here during asan.exp testing.
> You can very easily either install and enable prelink on one of your
> x86_64-linux testing boxes, or just install it and add test that
> will say prelink -r 0x3600000000 some test shared library and then
> just use it in sanitized program (that will also verify that you can mmap
> libraries in that range), or even just write a test that will in a
> non-instrumented ctor with lower priority than asan's priority
> mmap a few pages at 0x3000000000 and close to 0x3fffff0000
> and store some data into those buffers later on in sanitized code.

I forgot you don't even need prelink -r 0x3600000000 for the testing, you
can just link it as
$(CXX) -shared -Wl,-Ttext-segment=0x3600000000 -fPIC -o testlib.so -fsanitize=address testlib.C
So, put some asan tests into the executable, some tests into the shared
library and link the executable against the shared library placed in the
area where prelink allocates addresses to shared libraries.
Perhaps build 3 such libraries, one at 0x3000000000, one somewhere middle
of that range and one close to the end of the range.

	Jakub

The patch seems to work on a simple test. Let me digest it.
I am trying to understand if there are problems with it other than the
added complexity (which is what I don't like the most).

-Wl,-Ttext-segment=0x3600000000 does not work with binutils-gold.
gold understands -Wl,-Ttext=0x3600000000, but bfd ld doesn't.
Do you know any flag supported by both?

--kcc


On Thu, Feb 14, 2013 at 12:48 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Feb 13, 2013 at 04:19:14PM +0100, Jakub Jelinek wrote:
>> Here is the patch, works just fine for me here during asan.exp testing.
>> You can very easily either install and enable prelink on one of your
>> x86_64-linux testing boxes, or just install it and add test that
>> will say prelink -r 0x3600000000 some test shared library and then
>> just use it in sanitized program (that will also verify that you can mmap
>> libraries in that range), or even just write a test that will in a
>> non-instrumented ctor with lower priority than asan's priority
>> mmap a few pages at 0x3000000000 and close to 0x3fffff0000
>> and store some data into those buffers later on in sanitized code.
>
> I forgot you don't even need prelink -r 0x3600000000 for the testing, you
> can just link it as
> $(CXX) -shared -Wl,-Ttext-segment=0x3600000000 -fPIC -o testlib.so -fsanitize=address testlib.C
> So, put some asan tests into the executable, some tests into the shared
> library and link the executable against the shared library placed in the
> area where prelink allocates addresses to shared libraries.
> Perhaps build 3 such libraries, one at 0x3000000000, one somewhere middle
> of that range and one close to the end of the range.
>
>         Jakub

On Thu, Feb 14, 2013 at 03:55:47PM +0400, Konstantin Serebryany wrote:
> The patch seems to work on a simple test. Let me digest it.
> I am trying to understand if there are problems with it other than the
> added complexity (which is what I don't like the most).

Yes, it is some added complexity, but not too much, and something that can
be tested regularly that it works.

> -Wl,-Ttext-segment=0x3600000000 does not work with binutils-gold.
> gold understands -Wl,-Ttext=0x3600000000, but bfd ld doesn't.
> Do you know any flag supported by both?

-Wl,-Ttext is unfortunately something different, at least for
the bfd linker.  -Ttext-segment aligns the base of the whole shared library,
if you look at start of the linker script for -shared:
  /* Read-only sections, merged into text segment: */
  . = SEGMENT_START("text-segment", 0) + SIZEOF_HEADERS;
  .note.gnu.build-id : { *(.note.gnu.build-id) }
  .hash           : { *(.hash) }
  .gnu.hash       : { *(.gnu.hash) }
  .dynsym         : { *(.dynsym) }
  .dynstr         : { *(.dynstr) }
  .gnu.version    : { *(.gnu.version) }
  .gnu.version_d  : { *(.gnu.version_d) }
  .gnu.version_r  : { *(.gnu.version_r) }
...
  .rela.plt       :
    {
      *(.rela.plt)
      *(.rela.iplt)
    }
  .init           :
  {
    KEEP (*(.init))
  }
  .plt            : { *(.plt) *(.iplt) }
  .text           :
  {
    *(.text.unlikely .text.*_unlikely)
    *(.text.exit .text.exit.*)
-Ttext-segment chooses the base at which ELF headers will reside.
-Ttext aligns the .text section's start to that, so most likely the shared
library won't even link, because .init section will be many GBs appart from
.text section.

CCing Ian, if gold has any way to do something similar.
As I said, the alternative is to link the library normally, and run
prelink -r 0x3600000000 libtest.so on the shared library afterwards if prelink is
installed, and make sure you install it on your linux/x86-64 test boxes.

	Jakub

On Thu, Feb 14, 2013 at 4:19 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Feb 14, 2013 at 03:55:47PM +0400, Konstantin Serebryany wrote:
>> The patch seems to work on a simple test. Let me digest it.
>> I am trying to understand if there are problems with it other than the
>> added complexity (which is what I don't like the most).
>
> Yes, it is some added complexity, but not too much, and something that can
> be tested regularly that it works.
>
>> -Wl,-Ttext-segment=0x3600000000 does not work with binutils-gold.
>> gold understands -Wl,-Ttext=0x3600000000, but bfd ld doesn't.
>> Do you know any flag supported by both?
>
> -Wl,-Ttext is unfortunately something different, at least for
> the bfd linker.  -Ttext-segment aligns the base of the whole shared library,
> if you look at start of the linker script for -shared:
>   /* Read-only sections, merged into text segment: */
>   . = SEGMENT_START("text-segment", 0) + SIZEOF_HEADERS;
>   .note.gnu.build-id : { *(.note.gnu.build-id) }
>   .hash           : { *(.hash) }
>   .gnu.hash       : { *(.gnu.hash) }
>   .dynsym         : { *(.dynsym) }
>   .dynstr         : { *(.dynstr) }
>   .gnu.version    : { *(.gnu.version) }
>   .gnu.version_d  : { *(.gnu.version_d) }
>   .gnu.version_r  : { *(.gnu.version_r) }
> ...
>   .rela.plt       :
>     {
>       *(.rela.plt)
>       *(.rela.iplt)
>     }
>   .init           :
>   {
>     KEEP (*(.init))
>   }
>   .plt            : { *(.plt) *(.iplt) }
>   .text           :
>   {
>     *(.text.unlikely .text.*_unlikely)
>     *(.text.exit .text.exit.*)
> -Ttext-segment chooses the base at which ELF headers will reside.
> -Ttext aligns the .text section's start to that, so most likely the shared
> library won't even link, because .init section will be many GBs appart from
> .text section.
>
> CCing Ian, if gold has any way to do something similar.
> As I said, the alternative is to link the library normally, and run
> prelink -r 0x3600000000 libtest.so on the shared library afterwards if prelink is
> installed, and make sure you install it on your linux/x86-64 test boxes.

Another way is to simply force using the bfd linker (on ubuntu, when
gold is the default linker, there is still bfd linker under
/usr/bin/ld.bfd).
Still, better to have something that works for both linkers.

--kcc

>
>         Jakub

On Thu, Feb 14, 2013 at 4:19 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Feb 14, 2013 at 03:55:47PM +0400, Konstantin Serebryany wrote:
>> The patch seems to work on a simple test. Let me digest it.
>> I am trying to understand if there are problems with it other than the
>> added complexity (which is what I don't like the most).
>
> Yes, it is some added complexity, but not too much, and something that can
> be tested regularly that it works.

The complexity I am afraid of is not only in the code, but also at the
time of execution.
We and our users sometimes have to stare at the /proc/self/maps.
A mapping with 1 (ZeroBase) or 3 (default) asan sections is ok, but
6 extra asan sections becomes nearly incomprehensible, at least for me.

So, how about having kMidMemBeg as a variable, set as __asan_init.
Only if something is mapped around 0x003X00000000 we set it to non-zero.

http://llvm-reviews.chandlerc.com/D411 (still needs some cleanup)

Unfortunately, the test does not work if gold is the system linker.
Any suggestion on how to make the test work with either linker?

Thanks,

--kcc

>
>> -Wl,-Ttext-segment=0x3600000000 does not work with binutils-gold.
>> gold understands -Wl,-Ttext=0x3600000000, but bfd ld doesn't.
>> Do you know any flag supported by both?
>
> -Wl,-Ttext is unfortunately something different, at least for
> the bfd linker.  -Ttext-segment aligns the base of the whole shared library,
> if you look at start of the linker script for -shared:
>   /* Read-only sections, merged into text segment: */
>   . = SEGMENT_START("text-segment", 0) + SIZEOF_HEADERS;
>   .note.gnu.build-id : { *(.note.gnu.build-id) }
>   .hash           : { *(.hash) }
>   .gnu.hash       : { *(.gnu.hash) }
>   .dynsym         : { *(.dynsym) }
>   .dynstr         : { *(.dynstr) }
>   .gnu.version    : { *(.gnu.version) }
>   .gnu.version_d  : { *(.gnu.version_d) }
>   .gnu.version_r  : { *(.gnu.version_r) }
> ...
>   .rela.plt       :
>     {
>       *(.rela.plt)
>       *(.rela.iplt)
>     }
>   .init           :
>   {
>     KEEP (*(.init))
>   }
>   .plt            : { *(.plt) *(.iplt) }
>   .text           :
>   {
>     *(.text.unlikely .text.*_unlikely)
>     *(.text.exit .text.exit.*)
> -Ttext-segment chooses the base at which ELF headers will reside.
> -Ttext aligns the .text section's start to that, so most likely the shared
> library won't even link, because .init section will be many GBs appart from
> .text section.
>
> CCing Ian, if gold has any way to do something similar.
> As I said, the alternative is to link the library normally, and run
> prelink -r 0x3600000000 libtest.so on the shared library afterwards if prelink is
> installed, and make sure you install it on your linux/x86-64 test boxes.
>
>         Jakub

On Fri, Feb 15, 2013 at 11:45:15AM +0400, Konstantin Serebryany wrote:
> On Thu, Feb 14, 2013 at 4:19 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Thu, Feb 14, 2013 at 03:55:47PM +0400, Konstantin Serebryany wrote:
> >> The patch seems to work on a simple test. Let me digest it.
> >> I am trying to understand if there are problems with it other than the
> >> added complexity (which is what I don't like the most).
> >
> > Yes, it is some added complexity, but not too much, and something that can
> > be tested regularly that it works.
> 
> The complexity I am afraid of is not only in the code, but also at the
> time of execution.
> We and our users sometimes have to stare at the /proc/self/maps.
> A mapping with 1 (ZeroBase) or 3 (default) asan sections is ok, but
> 6 extra asan sections becomes nearly incomprehensible, at least for me.
> 
> So, how about having kMidMemBeg as a variable, set as __asan_init.
> Only if something is mapped around 0x003X00000000 we set it to non-zero.

That is fine for me.  Note that ASAN_FIXED_MAPPING 1 might not work well,
e.g. for shadow offset of 1ULL << 44, there the prelink library range falls
into the low memory and thus kMidMemBeg should be 0.

> http://llvm-reviews.chandlerc.com/D411 (still needs some cleanup)

One cleanup might be avoid calling MemoryRangeIsAvailable unnecessarily
too many times.  Guess if you move:
  uptr shadow_start = kLowShadowBeg;
  if (kLowShadowBeg) shadow_start -= GetMmapGranularity();
  uptr shadow_end = kHighShadowEnd;
  bool shadow_avail = MemoryRangeIsAvailable(shadow_start, shadow_end);
before the
#if ASAN_LINUX && defined(__x86_64__) && !ASAN_FIXED_MAPPING
  if (!MemoryRangeIsAvailable(kLowShadowBeg, kHighShadowEnd)) {
    kMidMemBeg = kLowMemEnd < 0x3000000000ULL ? 0x3000000000ULL : 0;
    kMidMemEnd = kLowMemEnd < 0x3000000000ULL ? 0x3fffffffffULL : 0;
  }
#endif
code, you can just use shadow_avail there and later.  Every
MemoryRangeIsAvailable reads /proc/self/maps again, right?

Also, on ppc64 the prelink library area is:
0x8001000000LL to 0x8100000000LL if we want to e.g. handle flexible mapping
there (perhaps better use 0x8000000000L as kMidMemBeg then), guess for 32-bit
architectures it is irrelevant, there is not much
point in using shadow offsets other than the default high one (which is high
enough) or 0 (then you need PIE, and thus prelink info is ignored both by
the kernel and dynamic linker).

> Unfortunately, the test does not work if gold is the system linker.
> Any suggestion on how to make the test work with either linker?

That is the question to Ian, if gold can do that at all.

As I said before, you can try something like:

#include <sys/mman.h>

struct A
{
  A ();
  void *ptr;
};

void *ptr;

__attribute__((no_address_safety_analysis))
A::A ()
{
  ptr = mmap ((void *) 0x3600000000UL, 65536, PROT_READ|PROT_WRITE,
	      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
}

A __attribute__((init_priority (1))) a;

int
main ()
{
  if (a.ptr != MAP_FAILED)
    {
      char *ptr = (char *) a.ptr;
      __asan_poison_memory_region (ptr, 4096);
      __asan_poison_memory_region (ptr + 61440, 4096);
      ptr[4096] = 23;
    }
}

and similar (and/or test that accessing the poisoned memory fails etc.).
For gcc you want to compile with -w, so that it doesn't warn about the
reserved init_priority, not sure what clang would do.

	Jakub

Ian, there is a question for you below.

On Fri, Feb 15, 2013 at 12:26 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Feb 15, 2013 at 11:45:15AM +0400, Konstantin Serebryany wrote:
>> On Thu, Feb 14, 2013 at 4:19 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Thu, Feb 14, 2013 at 03:55:47PM +0400, Konstantin Serebryany wrote:
>> >> The patch seems to work on a simple test. Let me digest it.
>> >> I am trying to understand if there are problems with it other than the
>> >> added complexity (which is what I don't like the most).
>> >
>> > Yes, it is some added complexity, but not too much, and something that can
>> > be tested regularly that it works.
>>
>> The complexity I am afraid of is not only in the code, but also at the
>> time of execution.
>> We and our users sometimes have to stare at the /proc/self/maps.
>> A mapping with 1 (ZeroBase) or 3 (default) asan sections is ok, but
>> 6 extra asan sections becomes nearly incomprehensible, at least for me.
>>
>> So, how about having kMidMemBeg as a variable, set as __asan_init.
>> Only if something is mapped around 0x003X00000000 we set it to non-zero.
>
> That is fine for me.  Note that ASAN_FIXED_MAPPING 1 might not work well,
> e.g. for shadow offset of 1ULL << 44, there the prelink library range falls
> into the low memory and thus kMidMemBeg should be 0.

Sure. ASAN_FIXED_MAPPING should be used for performance measurements
only -- this is not a release option.
(Added a more precise comment).

>
>> http://llvm-reviews.chandlerc.com/D411 (still needs some cleanup)
>
> One cleanup might be avoid calling MemoryRangeIsAvailable unnecessarily
> too many times.  Guess if you move:
>   uptr shadow_start = kLowShadowBeg;
>   if (kLowShadowBeg) shadow_start -= GetMmapGranularity();
>   uptr shadow_end = kHighShadowEnd;
>   bool shadow_avail = MemoryRangeIsAvailable(shadow_start, shadow_end);
> before the
> #if ASAN_LINUX && defined(__x86_64__) && !ASAN_FIXED_MAPPING
>   if (!MemoryRangeIsAvailable(kLowShadowBeg, kHighShadowEnd)) {
>     kMidMemBeg = kLowMemEnd < 0x3000000000ULL ? 0x3000000000ULL : 0;
>     kMidMemEnd = kLowMemEnd < 0x3000000000ULL ? 0x3fffffffffULL : 0;
>   }
> #endif
> code, you can just use shadow_avail there and later.  Every
> MemoryRangeIsAvailable reads /proc/self/maps again, right?

agree, done.

>
> Also, on ppc64 the prelink library area is:
> 0x8001000000LL to 0x8100000000LL if we want to e.g. handle flexible mapping
> there (perhaps better use 0x8000000000L as kMidMemBeg then), guess for 32-bit
> architectures it is irrelevant, there is not much
> point in using shadow offsets other than the default high one (which is high
> enough) or 0 (then you need PIE, and thus prelink info is ignored both by
> the kernel and dynamic linker).

Let's worry about ppc prelink separately.
We are not changing the shadow offset on ppc (because 7fff8000 does
not seem to help it anyway)
and no one complained so far.

>
>> Unfortunately, the test does not work if gold is the system linker.
>> Any suggestion on how to make the test work with either linker?
>
> That is the question to Ian, if gold can do that at all.

Yep.

>
> As I said before, you can try something like:
>
> #include <sys/mman.h>
>
> struct A
> {
>   A ();
>   void *ptr;
> };
>
> void *ptr;
>
> __attribute__((no_address_safety_analysis))
> A::A ()
> {
>   ptr = mmap ((void *) 0x3600000000UL, 65536, PROT_READ|PROT_WRITE,
>               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> }
>
> A __attribute__((init_priority (1))) a;
>
> int
> main ()
> {
>   if (a.ptr != MAP_FAILED)
>     {
>       char *ptr = (char *) a.ptr;
>       __asan_poison_memory_region (ptr, 4096);
>       __asan_poison_memory_region (ptr + 61440, 4096);
>       ptr[4096] = 23;
>     }
> }
>
> and similar (and/or test that accessing the poisoned memory fails etc.).
> For gcc you want to compile with -w, so that it doesn't warn about the
> reserved init_priority, not sure what clang would do.

This is ungood.
First, clang doesn't like it at all:
prelink1.cc:18:18: error: init_priority attribute requires integer
constant between 101 and 65535 inclusive
A __attribute__((init_priority (1))) a;

Second, in some settings we are using ASAN_USE_PREINIT_ARRAY=1
and this may become the default on linux at some point.
so, __asan_init will get called before A::A()

Waiting for Ian's reply about gold.

--kcc

>
>         Jakub

On Fri, Feb 15, 2013 at 12:47:30PM +0400, Konstantin Serebryany wrote:
> This is ungood.
> First, clang doesn't like it at all:
> prelink1.cc:18:18: error: init_priority attribute requires integer
> constant between 101 and 65535 inclusive
> A __attribute__((init_priority (1))) a;

For gcc it is just a warning, not error, so you can actually use it if you
know what you are doing.

Anyway, if gold doesn't have any way, you can always do the equivalent
of shell
which prelink 2>/dev/null && prelink -r 0x3600000000 libfoo.so
somewhere in the CMakeLists.txt.  That command doesn't affect system
libraries, can be run as normal user, and just transforms a library
from the default link state to -Wl,-Ttext-segment=0x3600000000
state (including debug info etc.).  You'd need to apt-get install prelink
or whatever command is for that on Ubuntu on the test boxes.

OT, unrelated thing, in include/asan_interface.h there is one
#if __has_feature(address_sanitizer)
which for GCC should better be:
#if (defined __has_feature && __has_feature(address_sanitizer)) \
    || defined(__SANITIZE_ADDRESS__)
(and similarly in asan_internal.h).

	Jakub

On Fri, Feb 15, 2013 at 1:05 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Feb 15, 2013 at 12:47:30PM +0400, Konstantin Serebryany wrote:
>> This is ungood.
>> First, clang doesn't like it at all:
>> prelink1.cc:18:18: error: init_priority attribute requires integer
>> constant between 101 and 65535 inclusive
>> A __attribute__((init_priority (1))) a;
>
> For gcc it is just a warning, not error, so you can actually use it if you
> know what you are doing.
>
> Anyway, if gold doesn't have any way, you can always do the equivalent
> of shell
> which prelink 2>/dev/null && prelink -r 0x3600000000 libfoo.so
> somewhere in the CMakeLists.txt.  That command doesn't affect system
> libraries, can be run as normal user, and just transforms a library
> from the default link state to -Wl,-Ttext-segment=0x3600000000
> state (including debug info etc.).  You'd need to apt-get install prelink
> or whatever command is for that on Ubuntu on the test boxes.

that's another option, not perfect though (someone not having prelink
on his/her box may break the tests w/o noticing).

>
> OT, unrelated thing, in include/asan_interface.h there is one
> #if __has_feature(address_sanitizer)
> which for GCC should better be:
> #if (defined __has_feature && __has_feature(address_sanitizer)) \
>     || defined(__SANITIZE_ADDRESS__)
> (and similarly in asan_internal.h).

z.c:1:44: error: missing binary operator before token "("
 #if (defined __has_feature && __has_feature(address_sanitizer)) \

This should be more like the code below

#if !defined(__has_feature)
#define __has_feature(x) 0
#endif
#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)

[hopefully not starting a holly war] Any chance to teach gcc/cpp about
__has_feature?

--kcc

>
>         Jakub

On Fri, Feb 15, 2013 at 01:30:18PM +0400, Konstantin Serebryany wrote:
> > OT, unrelated thing, in include/asan_interface.h there is one
> > #if __has_feature(address_sanitizer)
> > which for GCC should better be:
> > #if (defined __has_feature && __has_feature(address_sanitizer)) \
> >     || defined(__SANITIZE_ADDRESS__)
> > (and similarly in asan_internal.h).
> 
> z.c:1:44: error: missing binary operator before token "("
>  #if (defined __has_feature && __has_feature(address_sanitizer)) \
> 
> This should be more like the code below
> 
> #if !defined(__has_feature)
> #define __has_feature(x) 0
> #endif
> #if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)

I don't care much.  Would
#if (defined(__has_feature) && __has_feature(address_sanitizer)) \
    || defined(__SANITIZE_ADDRESS__)
work?  In any case, that looks like clang bug, if you have something that
behaves like a function-like macro, #if defined macro or #ifdef macro
or #if defined macro || 1 and similar should work just fine, only if
the macro is followed by ( it should behave differently.

> [hopefully not starting a holly war] Any chance to teach gcc/cpp about
> __has_feature?

Not for GCC 4.8, that is a new feature, so certainly too late for that.

	Jakub

On Fri, Feb 15, 2013 at 1:37 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Feb 15, 2013 at 01:30:18PM +0400, Konstantin Serebryany wrote:
>> > OT, unrelated thing, in include/asan_interface.h there is one
>> > #if __has_feature(address_sanitizer)
>> > which for GCC should better be:
>> > #if (defined __has_feature && __has_feature(address_sanitizer)) \
>> >     || defined(__SANITIZE_ADDRESS__)
>> > (and similarly in asan_internal.h).
>>
>> z.c:1:44: error: missing binary operator before token "("
>>  #if (defined __has_feature && __has_feature(address_sanitizer)) \
>>
>> This should be more like the code below
>>
>> #if !defined(__has_feature)
>> #define __has_feature(x) 0
>> #endif
>> #if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
>
> I don't care much.  Would
> #if (defined(__has_feature) && __has_feature(address_sanitizer)) \
>     || defined(__SANITIZE_ADDRESS__)
> work?  In any case, that looks like clang bug, if you have something that
> behaves like a function-like macro, #if defined macro or #ifdef macro
> or #if defined macro || 1 and similar should work just fine, only if
> the macro is followed by ( it should behave differently.

That's gcc:

% cat z.c
#if (defined __has_feature && __has_feature(address_sanitizer)) \
        || defined(__SANITIZE_ADDRESS__)
#error OK
#else
#error NOOOOO
#endif
% gcc -c -fsanitize=address  z.c
z.c:1:44: error: missing binary operator before token "("
 #if (defined __has_feature && __has_feature(address_sanitizer)) \
                                            ^
z.c:5:2: error: #error NOOOOO
 #error NOOOOO
  ^
% clang -c -fsanitize=address  z.c
z.c:3:2: error: OK
#error OK
 ^
1 error generated.
%


>
>> [hopefully not starting a holly war] Any chance to teach gcc/cpp about
>> __has_feature?
>
> Not for GCC 4.8, that is a new feature, so certainly too late for that.

How about 4.9?

>
>         Jakub

I've submitted http://llvm.org/viewvc/llvm-project?view=revision&revision=175263
If it survives a few days of testing I'll do another merge to gcc.

--kcc

On Fri, Feb 15, 2013 at 1:47 PM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
> On Fri, Feb 15, 2013 at 1:37 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Fri, Feb 15, 2013 at 01:30:18PM +0400, Konstantin Serebryany wrote:
>>> > OT, unrelated thing, in include/asan_interface.h there is one
>>> > #if __has_feature(address_sanitizer)
>>> > which for GCC should better be:
>>> > #if (defined __has_feature && __has_feature(address_sanitizer)) \
>>> >     || defined(__SANITIZE_ADDRESS__)
>>> > (and similarly in asan_internal.h).
>>>
>>> z.c:1:44: error: missing binary operator before token "("
>>>  #if (defined __has_feature && __has_feature(address_sanitizer)) \
>>>
>>> This should be more like the code below
>>>
>>> #if !defined(__has_feature)
>>> #define __has_feature(x) 0
>>> #endif
>>> #if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
>>
>> I don't care much.  Would
>> #if (defined(__has_feature) && __has_feature(address_sanitizer)) \
>>     || defined(__SANITIZE_ADDRESS__)
>> work?  In any case, that looks like clang bug, if you have something that
>> behaves like a function-like macro, #if defined macro or #ifdef macro
>> or #if defined macro || 1 and similar should work just fine, only if
>> the macro is followed by ( it should behave differently.
>
> That's gcc:
>
> % cat z.c
> #if (defined __has_feature && __has_feature(address_sanitizer)) \
>         || defined(__SANITIZE_ADDRESS__)
> #error OK
> #else
> #error NOOOOO
> #endif
> % gcc -c -fsanitize=address  z.c
> z.c:1:44: error: missing binary operator before token "("
>  #if (defined __has_feature && __has_feature(address_sanitizer)) \
>                                             ^
> z.c:5:2: error: #error NOOOOO
>  #error NOOOOO
>   ^
> % clang -c -fsanitize=address  z.c
> z.c:3:2: error: OK
> #error OK
>  ^
> 1 error generated.
> %
>
>
>>
>>> [hopefully not starting a holly war] Any chance to teach gcc/cpp about
>>> __has_feature?
>>
>> Not for GCC 4.8, that is a new feature, so certainly too late for that.
>
> How about 4.9?
>
>>
>>         Jakub

On Thu, Feb 14, 2013 at 11:45 PM, Konstantin Serebryany
<konstantin.s.serebryany@gmail.com> wrote:
>
> Unfortunately, the test does not work if gold is the system linker.
> Any suggestion on how to make the test work with either linker?

I don't know of a way to set the address of the text segment for both
GNU ld and gold.  As you have observed, GNU ld's -Ttext-segment option
is the same as gold's -Ttext option.  GNU ld's -Ttext option is
useless on ELF systems.

I will add -Ttext-segment to gold as an alias for -Ttext, but that
won't help you today.

Sorry.

Ian

On Fri, Feb 15, 2013 at 07:39:28AM -0800, Ian Lance Taylor wrote:
> On Thu, Feb 14, 2013 at 11:45 PM, Konstantin Serebryany
> <konstantin.s.serebryany@gmail.com> wrote:
> >
> > Unfortunately, the test does not work if gold is the system linker.
> > Any suggestion on how to make the test work with either linker?
> 
> I don't know of a way to set the address of the text segment for both
> GNU ld and gold.  As you have observed, GNU ld's -Ttext-segment option
> is the same as gold's -Ttext option.  GNU ld's -Ttext option is
> useless on ELF systems.
> 
> I will add -Ttext-segment to gold as an alias for -Ttext, but that
> won't help you today.

Can't you use then something like:
// RUN: %clangxx_asan -m64 -DBUILD_SO=1 -fPIC -shared %s -o %t.so -Wl,-Ttext-segment=0x3600000000 || %clangxx_asan -m64 -DBUILD_SO=1 -fPIC -shared %s -o %t.so -Wl,-Ttext=0x3600000000 || exit 0

?

	Jakub

On Mon, Feb 18, 2013 at 12:20 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Feb 15, 2013 at 07:39:28AM -0800, Ian Lance Taylor wrote:
>> On Thu, Feb 14, 2013 at 11:45 PM, Konstantin Serebryany
>> <konstantin.s.serebryany@gmail.com> wrote:
>> >
>> > Unfortunately, the test does not work if gold is the system linker.
>> > Any suggestion on how to make the test work with either linker?
>>
>> I don't know of a way to set the address of the text segment for both
>> GNU ld and gold.  As you have observed, GNU ld's -Ttext-segment option
>> is the same as gold's -Ttext option.  GNU ld's -Ttext option is
>> useless on ELF systems.
>>
>> I will add -Ttext-segment to gold as an alias for -Ttext, but that
>> won't help you today.
>
> Can't you use then something like:
> // RUN: %clangxx_asan -m64 -DBUILD_SO=1 -fPIC -shared %s -o %t.so -Wl,-Ttext-segment=0x3600000000 || %clangxx_asan -m64 -DBUILD_SO=1 -fPIC -shared %s -o %t.so -Wl,-Ttext=0x3600000000 || exit 0

Yep, that's what I was going to do once Ian confirmed that -Ttext in
gold is equivalent to Ttext-segment bfd ld.
http://llvm.org/viewvc/llvm-project?rev=175431&view=rev

Thanks!

--kcc


>
> ?
>
>         Jakub

On Fri, Feb 15, 2013 at 12:47:30PM +0400, Konstantin Serebryany wrote:
> Sure. ASAN_FIXED_MAPPING should be used for performance measurements
> only -- this is not a release option.
> (Added a more precise comment).

BTW, today I think I've discovered what looks like a prelink bug,
but perhaps we need to bump kMidMemEnd a little bit.
With prelink -R, the libraries are randomized in the selected range of addresses,
by picking a random number in between the boundaries of the address range and first
assigning addresses to libraries above that random offset and when there is no longer any room
above it, starting from the beginning of the range.  Unfortunately the code seems to have
some issues if the random address is chosen close to the end of the range, then in some
cases some libraries could start before the range, but end after the range.

On one of my boxes there is (only 4 libraries out of 822 have this problem,
only discovered it because gdb is linked against one of them and I've tried to
LD_PRELOAD=libasan.so.0 to gdb so that the debugged program would inherit that):

prelink -pv 2>&1 | grep 0x0000003.*-.*0x0000004
/usr/lib64/libgmlib.so.1.0.7 [0x1ea5b3cf] 0x0000003fffe00000-0x0000004000008460:
/usr/lib64/libiec61883.so.0.1.1 [0x56363ffc] 0x0000003fffe00000-0x000000400000c3e0:
/usr/lib64/libncurses.so.5.9 [0x120e1b8a] 0x0000003fffe00000-0x0000004000023860:
/usr/lib64/libsoup-2.4.so.1.5.0 [0x99ff4d51] 0x0000003fffe00000-0x000000400006a258:

while on others none.

So, can kMidMemEnd be slightly incremented above this?  Either 0x4fffffffffULL,
or 0x40ffffffffULL (or replace that 0f with 1f, 2f, etc.).
Small model shared libraries can be only up to 2GB in size, so even
0x40ffffffffULL should be big enough for most cases, but 0x4fffffffffULL
could be even safer.  I've justed tested and 0x4fffffffffULL results in
|| `[0x10007fff8000, 0x7fffffffffff]` || HighMem    ||
|| `[0x02008fff7000, 0x10007fff7fff]` || HighShadow ||
|| `[0x005000000000, 0x02008fff6fff]` || ShadowGap3 ||
|| `[0x003000000000, 0x004fffffffff]` || MidMem     ||
|| `[0x000a7fff8000, 0x002fffffffff]` || ShadowGap2 ||
|| `[0x00067fff8000, 0x000a7fff7fff]` || MidShadow  ||
|| `[0x00008fff7000, 0x00067fff7fff]` || ShadowGap  ||
|| `[0x00007fff8000, 0x00008fff6fff]` || LowShadow  ||
|| `[0x000000000000, 0x00007fff7fff]` || LowMem     ||
MemToShadow(shadow): 0x00008fff7000 0x000091ff6dff 0x004091ff6e00 0x02008fff6fff 0x00014fff7000 0x0001cfff6fff
and seems to work just fine.

	Jakub

On Fri, Feb 22, 2013 at 8:32 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Feb 15, 2013 at 12:47:30PM +0400, Konstantin Serebryany wrote:
>> Sure. ASAN_FIXED_MAPPING should be used for performance measurements
>> only -- this is not a release option.
>> (Added a more precise comment).
>
> BTW, today I think I've discovered what looks like a prelink bug,
> but perhaps we need to bump kMidMemEnd a little bit.
> With prelink -R, the libraries are randomized in the selected range of addresses,
> by picking a random number in between the boundaries of the address range and first
> assigning addresses to libraries above that random offset and when there is no longer any room
> above it, starting from the beginning of the range.  Unfortunately the code seems to have
> some issues if the random address is chosen close to the end of the range, then in some
> cases some libraries could start before the range, but end after the range.
>
> On one of my boxes there is (only 4 libraries out of 822 have this problem,
> only discovered it because gdb is linked against one of them and I've tried to
> LD_PRELOAD=libasan.so.0 to gdb so that the debugged program would inherit that):
>
> prelink -pv 2>&1 | grep 0x0000003.*-.*0x0000004
> /usr/lib64/libgmlib.so.1.0.7 [0x1ea5b3cf] 0x0000003fffe00000-0x0000004000008460:
> /usr/lib64/libiec61883.so.0.1.1 [0x56363ffc] 0x0000003fffe00000-0x000000400000c3e0:
> /usr/lib64/libncurses.so.5.9 [0x120e1b8a] 0x0000003fffe00000-0x0000004000023860:
> /usr/lib64/libsoup-2.4.so.1.5.0 [0x99ff4d51] 0x0000003fffe00000-0x000000400006a258:
>
> while on others none.
>
> So, can kMidMemEnd be slightly incremented above this?  Either 0x4fffffffffULL,
> or 0x40ffffffffULL (or replace that 0f with 1f, 2f, etc.).
> Small model shared libraries can be only up to 2GB in size, so even
> 0x40ffffffffULL should be big enough for most cases, but 0x4fffffffffULL
> could be even safer.  I've justed tested and 0x4fffffffffULL results in
> || `[0x10007fff8000, 0x7fffffffffff]` || HighMem    ||
> || `[0x02008fff7000, 0x10007fff7fff]` || HighShadow ||
> || `[0x005000000000, 0x02008fff6fff]` || ShadowGap3 ||
> || `[0x003000000000, 0x004fffffffff]` || MidMem     ||
> || `[0x000a7fff8000, 0x002fffffffff]` || ShadowGap2 ||
> || `[0x00067fff8000, 0x000a7fff7fff]` || MidShadow  ||
> || `[0x00008fff7000, 0x00067fff7fff]` || ShadowGap  ||
> || `[0x00007fff8000, 0x00008fff6fff]` || LowShadow  ||
> || `[0x000000000000, 0x00007fff7fff]` || LowMem     ||
> MemToShadow(shadow): 0x00008fff7000 0x000091ff6dff 0x004091ff6e00 0x02008fff6fff 0x00014fff7000 0x0001cfff6fff
> and seems to work just fine.

I am sorry, I missed this message.
Indeed, the change looks safe,
http://llvm.org/viewvc/llvm-project?rev=176250&view=rev

Thanks!

--kcc

>
>         Jakub