Re: [PATCH 0/7] libsanitizer: merge from trunk
On Wed, Oct 31, 2018 at 12:24:47PM +0100, Martin Liška wrote: > I install the patch set. If I'm correct one last missing piece should > be update of LOCAL_PATCHES. I'm sending patch for it. Ok, thanks. Jakub
Re: [PATCH 0/7] libsanitizer: merge from trunk
Hi. I install the patch set. If I'm correct one last missing piece should be update of LOCAL_PATCHES. I'm sending patch for it. Ready for trunk? Thanks, Martin >From 02134e26743eed447f62f7e22d75ddfe605e88e3 Mon Sep 17 00:00:00 2001 From: marxin Date: Wed, 31 Oct 2018 12:22:36 +0100 Subject: [PATCH] Update LOCAL_PATCHES after libsanitizer merge. libsanitizer/ChangeLog: 2018-10-31 Martin Liska * LOCAL_PATCHES: Update to installed revisions. --- libsanitizer/LOCAL_PATCHES | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/libsanitizer/LOCAL_PATCHES b/libsanitizer/LOCAL_PATCHES index 69544c33a89..56e74b8b73a 100644 --- a/libsanitizer/LOCAL_PATCHES +++ b/libsanitizer/LOCAL_PATCHES @@ -1,4 +1,3 @@ -r241980 -r241981 -r242478 -r242633 +r265667 +r265668 +r265669 -- 2.19.0
Re: [PATCH 0/7] libsanitizer: merge from trunk
On Mon, Oct 29, 2018 at 05:00:36PM +0100, Jakub Jelinek wrote: > On Mon, Oct 29, 2018 at 10:38:11AM -0500, Bill Seurer wrote: > > > I'm still wondering what didn't work with 41 bits? AFAICS, due to > > > highshadow=highmem-offset and lowshadow=low+offset, and the existence of a > > > non-empty shadow-gap, offset must be minimum(vbits)-3 (vbits being one of > > > the above numbers). Why would 41 not work for the other vbit setting? > > > It would lead to a large shadow gap, but so? If a shadow-gap isn't > > > necessary then minimum(vbits)-2 would also work. > > > > 41 was what the value previously was and it did not work (IIRC with 47 bit > > VMA kernels which were "new" at the time) thus the change. > > What we'd like to see is details or URL with those why it didn't work with > 47 bit. E.g. x86_64 uses much lower shadow offset (0x7fff8000), asan has > support for multiple shadow areas, gaps and normal areas. > I think ppc64 binaries are usually using 0x1000 base, so that isn't a > problem, so is there a conflict with the default dynamic linker placement or > default placement of shared libraries when using the 47-bit VMA? > What do you get with the 41-bit shadow offset and ASAN_OPTIONS=verbosity=1 > on 47-bit VMA that e.g. kMidMemBeg/kMidMemEnd couldn't solve? Thanks to Bill for debugging last night; my understanding is that the 1UL<<41 to 1UL<<44 shadow offset change was an attempt to fix the issue that Martin fixed on the gcc side (and in llvm too) with changing PPC64 kAllocatorSpace from 0xa00ULL to ~(uptr)0. Sowe can now safely go back to 1UL<<41 shadow offset, which will work also with 44-bit VA. That setting is known to work with 44-bit, 46-bit and 47-bit virtual address space, we don't know if there are any kernels out there with the higher address space sizes the recent 4.13+ and 4.18+ kernel sources suggest (49-bit and 52-bit). Guess we'll need to talk to some kernel people. Anyway, for the gcc backport, I'd suggest to revert the 41->44 changes and don't bump libasan soname. The patch set LGTM with those changes. Jakub
Re: [PATCH 0/7] libsanitizer: merge from trunk
On Mon, Oct 29, 2018 at 10:38:11AM -0500, Bill Seurer wrote: > > I'm still wondering what didn't work with 41 bits? AFAICS, due to > > highshadow=highmem-offset and lowshadow=low+offset, and the existence of a > > non-empty shadow-gap, offset must be minimum(vbits)-3 (vbits being one of > > the above numbers). Why would 41 not work for the other vbit setting? > > It would lead to a large shadow gap, but so? If a shadow-gap isn't > > necessary then minimum(vbits)-2 would also work. > > 41 was what the value previously was and it did not work (IIRC with 47 bit > VMA kernels which were "new" at the time) thus the change. What we'd like to see is details or URL with those why it didn't work with 47 bit. E.g. x86_64 uses much lower shadow offset (0x7fff8000), asan has support for multiple shadow areas, gaps and normal areas. I think ppc64 binaries are usually using 0x1000 base, so that isn't a problem, so is there a conflict with the default dynamic linker placement or default placement of shared libraries when using the 47-bit VMA? What do you get with the 41-bit shadow offset and ASAN_OPTIONS=verbosity=1 on 47-bit VMA that e.g. kMidMemBeg/kMidMemEnd couldn't solve? Jakub
Re: [PATCH 0/7] libsanitizer: merge from trunk
On 10/29/18 10:26, Michael Matz wrote:
Hi,
On Mon, 29 Oct 2018, Bill Seurer wrote:
Just for the record: am I right that any system using 44 bit of VMA will
fail because
anything + (1 << 44) will be out of process address space?
Yes.
And I noticed that documentation in sanitizer_linux.cc is misleading:
...
uptr GetMaxVirtualAddress() {
#if (SANITIZER_NETBSD || SANITIZER_OPENBSD) && defined(__x86_64__)
return 0x7f7ff000ULL; // (0x7f80 - PAGE_SIZE)
#elif SANITIZER_WORDSIZE == 64
# if defined(__powerpc64__) || defined(__aarch64__)
// On PowerPC64 we have two different address space layouts: 44- and
46-bit.
// We somehow need to figure out which one we are using now and choose
// one of 0x0fffUL and 0x3fffUL.
...
That should be adjusted.
Apparently for ppc64 there are many different layouts now, 44, 46, 47, 49,
52
at least depending on which kernel and page size you choose.
So, ideally we want some choice that works with all of them. Shadow offset
1ULL<<44 is not that choice.
We (llvm team) tried to get it to work on all the different kernels but didn't
find anything that worked. Finally we just went with a value that worked on
the newer kernels as the 44 bit one was not a target of concern.
I'm still wondering what didn't work with 41 bits? AFAICS, due to
highshadow=highmem-offset and lowshadow=low+offset, and the existence of a
non-empty shadow-gap, offset must be minimum(vbits)-3 (vbits being one of
the above numbers). Why would 41 not work for the other vbit setting?
It would lead to a large shadow gap, but so? If a shadow-gap isn't
necessary then minimum(vbits)-2 would also work.
41 was what the value previously was and it did not work (IIRC with 47
bit VMA kernels which were "new" at the time) thus the change.
If someone comes up with something that they think will work I am
willing to test it with both llvm and gcc.
--
-Bill Seurer
Re: [PATCH 0/7] libsanitizer: merge from trunk
Hi,
On Mon, 29 Oct 2018, Bill Seurer wrote:
> >> Just for the record: am I right that any system using 44 bit of VMA will
> >> fail because
> >> anything + (1 << 44) will be out of process address space?
> >
> > Yes.
> >
> >> And I noticed that documentation in sanitizer_linux.cc is misleading:
> >>
> >> ...
> >> uptr GetMaxVirtualAddress() {
> >> #if (SANITIZER_NETBSD || SANITIZER_OPENBSD) && defined(__x86_64__)
> >>return 0x7f7ff000ULL; // (0x7f80 - PAGE_SIZE)
> >> #elif SANITIZER_WORDSIZE == 64
> >> # if defined(__powerpc64__) || defined(__aarch64__)
> >>// On PowerPC64 we have two different address space layouts: 44- and
> >>46-bit.
> >>// We somehow need to figure out which one we are using now and choose
> >>// one of 0x0fffUL and 0x3fffUL.
> >> ...
> >>
> >> That should be adjusted.
> >
> > Apparently for ppc64 there are many different layouts now, 44, 46, 47, 49,
> > 52
> > at least depending on which kernel and page size you choose.
> > So, ideally we want some choice that works with all of them. Shadow offset
> > 1ULL<<44 is not that choice.
>
> We (llvm team) tried to get it to work on all the different kernels but didn't
> find anything that worked. Finally we just went with a value that worked on
> the newer kernels as the 44 bit one was not a target of concern.
I'm still wondering what didn't work with 41 bits? AFAICS, due to
highshadow=highmem-offset and lowshadow=low+offset, and the existence of a
non-empty shadow-gap, offset must be minimum(vbits)-3 (vbits being one of
the above numbers). Why would 41 not work for the other vbit setting?
It would lead to a large shadow gap, but so? If a shadow-gap isn't
necessary then minimum(vbits)-2 would also work.
Ciao,
Michael.
Re: [PATCH 0/7] libsanitizer: merge from trunk
On 10/29/18 06:24, Jakub Jelinek wrote:
On Mon, Oct 29, 2018 at 12:13:04PM +0100, Martin Liška wrote:
Just for the record: am I right that any system using 44 bit of VMA will fail
because
anything + (1 << 44) will be out of process address space?
Yes.
And I noticed that documentation in sanitizer_linux.cc is misleading:
...
uptr GetMaxVirtualAddress() {
#if (SANITIZER_NETBSD || SANITIZER_OPENBSD) && defined(__x86_64__)
return 0x7f7ff000ULL; // (0x7f80 - PAGE_SIZE)
#elif SANITIZER_WORDSIZE == 64
# if defined(__powerpc64__) || defined(__aarch64__)
// On PowerPC64 we have two different address space layouts: 44- and 46-bit.
// We somehow need to figure out which one we are using now and choose
// one of 0x0fffUL and 0x3fffUL.
...
That should be adjusted.
Apparently for ppc64 there are many different layouts now, 44, 46, 47, 49, 52
at least depending on which kernel and page size you choose.
So, ideally we want some choice that works with all of them. Shadow offset
1ULL<<44 is not that choice.
We (llvm team) tried to get it to work on all the different kernels but
didn't find anything that worked. Finally we just went with a value
that worked on the newer kernels as the 44 bit one was not a target of
concern.
--
-Bill Seurer
Re: [PATCH 0/7] libsanitizer: merge from trunk
On Mon, Oct 29, 2018 at 12:13:04PM +0100, Martin Liška wrote:
> Just for the record: am I right that any system using 44 bit of VMA will fail
> because
> anything + (1 << 44) will be out of process address space?
Yes.
> And I noticed that documentation in sanitizer_linux.cc is misleading:
>
> ...
> uptr GetMaxVirtualAddress() {
> #if (SANITIZER_NETBSD || SANITIZER_OPENBSD) && defined(__x86_64__)
> return 0x7f7ff000ULL; // (0x7f80 - PAGE_SIZE)
> #elif SANITIZER_WORDSIZE == 64
> # if defined(__powerpc64__) || defined(__aarch64__)
> // On PowerPC64 we have two different address space layouts: 44- and 46-bit.
> // We somehow need to figure out which one we are using now and choose
> // one of 0x0fffUL and 0x3fffUL.
> ...
>
> That should be adjusted.
Apparently for ppc64 there are many different layouts now, 44, 46, 47, 49, 52
at least depending on which kernel and page size you choose.
So, ideally we want some choice that works with all of them. Shadow offset
1ULL<<44 is not that choice.
Jakub
Re: [PATCH 0/7] libsanitizer: merge from trunk
On 10/26/18 4:52 PM, Jakub Jelinek wrote:
> On Fri, Oct 26, 2018 at 09:48:54AM -0500, Bill Seurer wrote:
>> On 10/26/18 03:57, Jakub Jelinek wrote:
>>> On Thu, Oct 25, 2018 at 12:49:42PM +0200, Jakub Jelinek wrote:
On Thu, Oct 25, 2018 at 12:15:46PM +0200, marxin wrote:
> I've just finished my first merge from libsanitizer mainline. Overall it
> looks fine, apparently ABI hasn't changed and so that SONAME bump is not
> needed.
Given the 6/7 patch, I think you need to bump libasan soname (it would be
weird to bump it on powerpc64* only).
>>>
>>> BTW, how can shadow offset be 1UL<<44 on powerpc64? That seems like they
>>> don't want to support anything but very recent kernels.
>>> E.g. looking at Linux 3.4 arch/powerpc/include/asm/processor.h
>>> I see
>>> /* 64-bit user address space is 44-bits (16TB user VM) */
>>> #define TASK_SIZE_USER64 (0x1000UL)
>>> so, the new choice must be incompatible with lots of kernels out there.
>>> Move recent kernels have:
>>> #define TASK_SIZE_64TB (0x4000UL)
>>> #define TASK_SIZE_128TB (0x8000UL)
>>> #define TASK_SIZE_512TB (0x0002UL)
>>> #define TASK_SIZE_1PB (0x0004UL)
>>> #define TASK_SIZE_2PB (0x0008UL)
>>> #define TASK_SIZE_4PB (0x0010UL)
>>> but 4.15 still tops at 512TB, 4.10 has just 64TB as the only choice, 3.8 as
>>> well.
>>>
>>> CCing Bill as he made this change.
>>>
>>> Jakub
>>>
>>
>> At the time for llvm the concern was to get it to work on newer kernels and
>> not worry (much) about the older ones. I did spend some time trying to get
>> it to work for both.
>
> Which exact task size doesn't work if shadow offset is 2TB and why?
>
> Jakub
>
Just for the record: am I right that any system using 44 bit of VMA will fail
because
anything + (1 << 44) will be out of process address space?
And I noticed that documentation in sanitizer_linux.cc is misleading:
...
uptr GetMaxVirtualAddress() {
#if (SANITIZER_NETBSD || SANITIZER_OPENBSD) && defined(__x86_64__)
return 0x7f7ff000ULL; // (0x7f80 - PAGE_SIZE)
#elif SANITIZER_WORDSIZE == 64
# if defined(__powerpc64__) || defined(__aarch64__)
// On PowerPC64 we have two different address space layouts: 44- and 46-bit.
// We somehow need to figure out which one we are using now and choose
// one of 0x0fffUL and 0x3fffUL.
...
That should be adjusted.
Thanks,
Martin
Re: [PATCH 0/7] libsanitizer: merge from trunk
On Fri, Oct 26, 2018 at 09:48:54AM -0500, Bill Seurer wrote: > On 10/26/18 03:57, Jakub Jelinek wrote: > > On Thu, Oct 25, 2018 at 12:49:42PM +0200, Jakub Jelinek wrote: > > > On Thu, Oct 25, 2018 at 12:15:46PM +0200, marxin wrote: > > > > I've just finished my first merge from libsanitizer mainline. Overall it > > > > looks fine, apparently ABI hasn't changed and so that SONAME bump is not > > > > needed. > > > > > > Given the 6/7 patch, I think you need to bump libasan soname (it would be > > > weird to bump it on powerpc64* only). > > > > BTW, how can shadow offset be 1UL<<44 on powerpc64? That seems like they > > don't want to support anything but very recent kernels. > > E.g. looking at Linux 3.4 arch/powerpc/include/asm/processor.h > > I see > > /* 64-bit user address space is 44-bits (16TB user VM) */ > > #define TASK_SIZE_USER64 (0x1000UL) > > so, the new choice must be incompatible with lots of kernels out there. > > Move recent kernels have: > > #define TASK_SIZE_64TB (0x4000UL) > > #define TASK_SIZE_128TB (0x8000UL) > > #define TASK_SIZE_512TB (0x0002UL) > > #define TASK_SIZE_1PB (0x0004UL) > > #define TASK_SIZE_2PB (0x0008UL) > > #define TASK_SIZE_4PB (0x0010UL) > > but 4.15 still tops at 512TB, 4.10 has just 64TB as the only choice, 3.8 as > > well. > > > > CCing Bill as he made this change. > > > > Jakub > > > > At the time for llvm the concern was to get it to work on newer kernels and > not worry (much) about the older ones. I did spend some time trying to get > it to work for both. Which exact task size doesn't work if shadow offset is 2TB and why? Jakub
Re: [PATCH 0/7] libsanitizer: merge from trunk
On 10/26/18 03:57, Jakub Jelinek wrote: On Thu, Oct 25, 2018 at 12:49:42PM +0200, Jakub Jelinek wrote: On Thu, Oct 25, 2018 at 12:15:46PM +0200, marxin wrote: I've just finished my first merge from libsanitizer mainline. Overall it looks fine, apparently ABI hasn't changed and so that SONAME bump is not needed. Given the 6/7 patch, I think you need to bump libasan soname (it would be weird to bump it on powerpc64* only). BTW, how can shadow offset be 1UL<<44 on powerpc64? That seems like they don't want to support anything but very recent kernels. E.g. looking at Linux 3.4 arch/powerpc/include/asm/processor.h I see /* 64-bit user address space is 44-bits (16TB user VM) */ #define TASK_SIZE_USER64 (0x1000UL) so, the new choice must be incompatible with lots of kernels out there. Move recent kernels have: #define TASK_SIZE_64TB (0x4000UL) #define TASK_SIZE_128TB (0x8000UL) #define TASK_SIZE_512TB (0x0002UL) #define TASK_SIZE_1PB (0x0004UL) #define TASK_SIZE_2PB (0x0008UL) #define TASK_SIZE_4PB (0x0010UL) but 4.15 still tops at 512TB, 4.10 has just 64TB as the only choice, 3.8 as well. CCing Bill as he made this change. Jakub At the time for llvm the concern was to get it to work on newer kernels and not worry (much) about the older ones. I did spend some time trying to get it to work for both. -- -Bill Seurer
Re: [PATCH 0/7] libsanitizer: merge from trunk
On Thu, Oct 25, 2018 at 12:49:42PM +0200, Jakub Jelinek wrote: > On Thu, Oct 25, 2018 at 12:15:46PM +0200, marxin wrote: > > I've just finished my first merge from libsanitizer mainline. Overall it > > looks fine, apparently ABI hasn't changed and so that SONAME bump is not > > needed. > > Given the 6/7 patch, I think you need to bump libasan soname (it would be > weird to bump it on powerpc64* only). BTW, how can shadow offset be 1UL<<44 on powerpc64? That seems like they don't want to support anything but very recent kernels. E.g. looking at Linux 3.4 arch/powerpc/include/asm/processor.h I see /* 64-bit user address space is 44-bits (16TB user VM) */ #define TASK_SIZE_USER64 (0x1000UL) so, the new choice must be incompatible with lots of kernels out there. Move recent kernels have: #define TASK_SIZE_64TB (0x4000UL) #define TASK_SIZE_128TB (0x8000UL) #define TASK_SIZE_512TB (0x0002UL) #define TASK_SIZE_1PB (0x0004UL) #define TASK_SIZE_2PB (0x0008UL) #define TASK_SIZE_4PB (0x0010UL) but 4.15 still tops at 512TB, 4.10 has just 64TB as the only choice, 3.8 as well. CCing Bill as he made this change. Jakub
Re: [PATCH 0/7] libsanitizer: merge from trunk
On 10/25/18 12:49 PM, Jakub Jelinek wrote: > On Thu, Oct 25, 2018 at 12:15:46PM +0200, marxin wrote: >> I've just finished my first merge from libsanitizer mainline. Overall it >> looks fine, apparently ABI hasn't changed and so that SONAME bump is not >> needed. > > Given the 6/7 patch, I think you need to bump libasan soname (it would be > weird to bump it on powerpc64* only). Ahh, you are exactly right! Let me do that in one more patch. Martin > > Jakub >
Re: [PATCH 0/7] libsanitizer: merge from trunk
On Thu, Oct 25, 2018 at 12:15:46PM +0200, marxin wrote: > I've just finished my first merge from libsanitizer mainline. Overall it > looks fine, apparently ABI hasn't changed and so that SONAME bump is not > needed. Given the 6/7 patch, I think you need to bump libasan soname (it would be weird to bump it on powerpc64* only). Jakub
