RFR [10] 8189800: Add support for AddressSanitizer

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR [10] 8189800: Add support for AddressSanitizer

Artem Smotrakov
Hello,

Please review the following patch which adds support for AddressSanitizer.

AddressSanitizer is a runtime memory error detector which looks for
various memory corruption issues and leaks.

Please refer to [1] for details. AddressSanitizer is available in gcc
4.8+ and clang 3.1+

The patch below introduces --enable-asan parameter for the configure
script which enables AddressSanitizer.

Bug: https://bugs.openjdk.java.net/browse/JDK-8189800
Webrev: http://cr.openjdk.java.net/~asmotrak/8189800/webrev.00/

[1] https://github.com/google/sanitizers/wiki/AddressSanitizer

Artem
Reply | Threaded
Open this post in threaded view
|

Re: RFR [10] 8189800: Add support for AddressSanitizer

Artem Smotrakov
cc'ing [hidden email] as David suggested.

Artem


On 10/27/2017 11:02 PM, Artem Smotrakov wrote:

> Hello,
>
> Please review the following patch which adds support for
> AddressSanitizer.
>
> AddressSanitizer is a runtime memory error detector which looks for
> various memory corruption issues and leaks.
>
> Please refer to [1] for details. AddressSanitizer is available in gcc
> 4.8+ and clang 3.1+
>
> The patch below introduces --enable-asan parameter for the configure
> script which enables AddressSanitizer.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189800
> Webrev: http://cr.openjdk.java.net/~asmotrak/8189800/webrev.00/
>
> [1] https://github.com/google/sanitizers/wiki/AddressSanitizer
>
> Artem

Reply | Threaded
Open this post in threaded view
|

Re: RFR [10] 8189800: Add support for AddressSanitizer

Magnus Ihse Bursie
On 2017-10-30 08:39, Artem Smotrakov wrote:

> cc'ing [hidden email] as David suggested.
>
> Artem
>
>
> On 10/27/2017 11:02 PM, Artem Smotrakov wrote:
>> Hello,
>>
>> Please review the following patch which adds support for
>> AddressSanitizer.
>>
>> AddressSanitizer is a runtime memory error detector which looks for
>> various memory corruption issues and leaks.
>>
>> Please refer to [1] for details. AddressSanitizer is available in gcc
>> 4.8+ and clang 3.1+
>>
>> The patch below introduces --enable-asan parameter for the configure
>> script which enables AddressSanitizer.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189800
>> Webrev: http://cr.openjdk.java.net/~asmotrak/8189800/webrev.00/
spec.gmk.in should only have export for variables that needs to be
exported in the environment for executing binaries, that is ASAN_OPTIONS
and LD_LIBRARY_PATH, not ASAN_ENABLED or DEVKIT_LIB_DIR.

I'm also a bit curious about the addition of of DEVKIT_LIB_DIR. Would
you care to elaborate your thinking?

Otherwise it looks good.

/Magnus

>>
>> [1] https://github.com/google/sanitizers/wiki/AddressSanitizer
>>
>> Artem
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR [10] 8189800: Add support for AddressSanitizer

Artem Smotrakov
Hi Magnus,

The current approach uses AddressSanitizer as a shared library
(libasan.so) which is part of GCC/Clang toolkit. In case you use system
toolkit, then libasan.so is available for linker and at runtime. But if
you set a custom toolkit by --with-devkit option, then libasan.so form
this toolkit may not be available for linker and at runtime by default.
As a result, you can get errors while linking and running. To fix that,
you normally need to make it available using ldconfig, or update
LD_LIBRARY_PATH. That's why it updates LD_LIBRARY_PATH with
DEVKIT_LIB_DIR if a custom toolkit was used. That may be helpful when
you build JDK in environment like jib/jprt.

I tried to remove exporting ASAN_ENABLED and DEVKIT_LIB_DIR, and as a
result, ASAN_OPTIONS and DEVKIT_LIB_DIR didn't go to jtreg command which
caused tests to fail when you run "make test". If we don't export
ASAN_OPTIONS and DEVKIT_LIB_DIR, then the updates in TestCommon.gmk
don't make much sense to me because those variables have to be
explicitly set for "make" anyway.

I can remove exporting those variables and revert TestCommon.gmk.
Although, it looks nicer to me if we can run the tests just with "make
test" without specifying ASAN_OPTIONS and DEVKIT_LIB_DIR explicitly.

What do you think?

Artem


On 10/30/2017 10:50 AM, Magnus Ihse Bursie wrote:

> On 2017-10-30 08:39, Artem Smotrakov wrote:
>> cc'ing [hidden email] as David suggested.
>>
>> Artem
>>
>>
>> On 10/27/2017 11:02 PM, Artem Smotrakov wrote:
>>> Hello,
>>>
>>> Please review the following patch which adds support for
>>> AddressSanitizer.
>>>
>>> AddressSanitizer is a runtime memory error detector which looks for
>>> various memory corruption issues and leaks.
>>>
>>> Please refer to [1] for details. AddressSanitizer is available in
>>> gcc 4.8+ and clang 3.1+
>>>
>>> The patch below introduces --enable-asan parameter for the configure
>>> script which enables AddressSanitizer.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189800
>>> Webrev: http://cr.openjdk.java.net/~asmotrak/8189800/webrev.00/
> spec.gmk.in should only have export for variables that needs to be
> exported in the environment for executing binaries, that is
> ASAN_OPTIONS and LD_LIBRARY_PATH, not ASAN_ENABLED or DEVKIT_LIB_DIR.
>
> I'm also a bit curious about the addition of of DEVKIT_LIB_DIR. Would
> you care to elaborate your thinking?
>
> Otherwise it looks good.
>
> /Magnus
>
>>>
>>> [1] https://github.com/google/sanitizers/wiki/AddressSanitizer
>>>
>>> Artem
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR [10] 8189800: Add support for AddressSanitizer

David Holmes
In reply to this post by Artem Smotrakov
Hi Artem,

On 28/10/2017 6:02 AM, Artem Smotrakov wrote:
> Hello,
>
> Please review the following patch which adds support for AddressSanitizer.
>
> AddressSanitizer is a runtime memory error detector which looks for
> various memory corruption issues and leaks.
>
> Please refer to [1] for details. AddressSanitizer is available in gcc
> 4.8+ and clang 3.1+

Should we be checking the version before adding the flags?

Thanks,
David

> The patch below introduces --enable-asan parameter for the configure
> script which enables AddressSanitizer.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8189800
> Webrev: http://cr.openjdk.java.net/~asmotrak/8189800/webrev.00/
>
> [1] https://github.com/google/sanitizers/wiki/AddressSanitizer
>
> Artem
Reply | Threaded
Open this post in threaded view
|

Re: RFR [10] 8189800: Add support for AddressSanitizer

Artem Smotrakov
Hi David,

That's a good question, I thought about it. According to [1]:

- recommended versions of gcc is 4.9.2
- the minimum accepted version of gcc is 4.7 (Older versions will
generate a warning by `configure` and are unlikely to work.)
- the minimum accepted version of clang is 3.2 (Older versions will not
be accepted by `configure`)

It looks like that clang has to be at least 3.2 which should contain
AddressSanitizer. Only for gcc, there may be a chance that someone wants
to use 4.7. So, we might want to check version to see if it's 4.7,
although I am not sure how many people would like to use gcc 4.7. As a
result, this case didn't look very common to me, so I preferred to
simplify the patch, and didn't add such a check.

Without version check, compilation is going to fail if gcc 4.7 is used,
and -fsanitize=address enabled.

[1]
http://hg.openjdk.java.net/jdk10/master/file/438e0c9f2f17/doc/building.md

Artem

On 10/31/2017 01:37 PM, David Holmes wrote:

> Hi Artem,
>
> On 28/10/2017 6:02 AM, Artem Smotrakov wrote:
>> Hello,
>>
>> Please review the following patch which adds support for
>> AddressSanitizer.
>>
>> AddressSanitizer is a runtime memory error detector which looks for
>> various memory corruption issues and leaks.
>>
>> Please refer to [1] for details. AddressSanitizer is available in gcc
>> 4.8+ and clang 3.1+
>
> Should we be checking the version before adding the flags?
>
> Thanks,
> David
>
>> The patch below introduces --enable-asan parameter for the configure
>> script which enables AddressSanitizer.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189800
>> Webrev: http://cr.openjdk.java.net/~asmotrak/8189800/webrev.00/
>>
>> [1] https://github.com/google/sanitizers/wiki/AddressSanitizer
>>
>> Artem

Reply | Threaded
Open this post in threaded view
|

Re: RFR [10] 8189800: Add support for AddressSanitizer

David Holmes
Sounds reasonable. Anyone using older gcc simply won't/shouldn't enable
Asan.

Thanks,
David

On 31/10/2017 8:58 PM, Artem Smotrakov wrote:

> Hi David,
>
> That's a good question, I thought about it. According to [1]:
>
> - recommended versions of gcc is 4.9.2
> - the minimum accepted version of gcc is 4.7 (Older versions will
> generate a warning by `configure` and are unlikely to work.)
> - the minimum accepted version of clang is 3.2 (Older versions will not
> be accepted by `configure`)
>
> It looks like that clang has to be at least 3.2 which should contain
> AddressSanitizer. Only for gcc, there may be a chance that someone wants
> to use 4.7. So, we might want to check version to see if it's 4.7,
> although I am not sure how many people would like to use gcc 4.7. As a
> result, this case didn't look very common to me, so I preferred to
> simplify the patch, and didn't add such a check.
>
> Without version check, compilation is going to fail if gcc 4.7 is used,
> and -fsanitize=address enabled.
>
> [1]
> http://hg.openjdk.java.net/jdk10/master/file/438e0c9f2f17/doc/building.md
>
> Artem
>
> On 10/31/2017 01:37 PM, David Holmes wrote:
>> Hi Artem,
>>
>> On 28/10/2017 6:02 AM, Artem Smotrakov wrote:
>>> Hello,
>>>
>>> Please review the following patch which adds support for
>>> AddressSanitizer.
>>>
>>> AddressSanitizer is a runtime memory error detector which looks for
>>> various memory corruption issues and leaks.
>>>
>>> Please refer to [1] for details. AddressSanitizer is available in gcc
>>> 4.8+ and clang 3.1+
>>
>> Should we be checking the version before adding the flags?
>>
>> Thanks,
>> David
>>
>>> The patch below introduces --enable-asan parameter for the configure
>>> script which enables AddressSanitizer.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189800
>>> Webrev: http://cr.openjdk.java.net/~asmotrak/8189800/webrev.00/
>>>
>>> [1] https://github.com/google/sanitizers/wiki/AddressSanitizer
>>>
>>> Artem
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR [10] 8189800: Add support for AddressSanitizer

Magnus Ihse Bursie
In reply to this post by Artem Smotrakov
On 2017-10-30 10:31, Artem Smotrakov wrote:

> Hi Magnus,
>
> The current approach uses AddressSanitizer as a shared library
> (libasan.so) which is part of GCC/Clang toolkit. In case you use
> system toolkit, then libasan.so is available for linker and at
> runtime. But if you set a custom toolkit by --with-devkit option, then
> libasan.so form this toolkit may not be available for linker and at
> runtime by default. As a result, you can get errors while linking and
> running. To fix that, you normally need to make it available using
> ldconfig, or update LD_LIBRARY_PATH. That's why it updates
> LD_LIBRARY_PATH with DEVKIT_LIB_DIR if a custom toolkit was used. That
> may be helpful when you build JDK in environment like jib/jprt.
>
> I tried to remove exporting ASAN_ENABLED and DEVKIT_LIB_DIR, and as a
> result, ASAN_OPTIONS and DEVKIT_LIB_DIR didn't go to jtreg command
> which caused tests to fail when you run "make test". If we don't
> export ASAN_OPTIONS and DEVKIT_LIB_DIR, then the updates in
> TestCommon.gmk don't make much sense to me because those variables
> have to be explicitly set for "make" anyway.
>
> I can remove exporting those variables and revert TestCommon.gmk.
> Although, it looks nicer to me if we can run the tests just with "make
> test" without specifying ASAN_OPTIONS and DEVKIT_LIB_DIR explicitly.
>
> What do you think?
Ah, I see. TestCommon.gmk is not properly integrated into the rest of
the build system. I'm still a bit surprised at this behavior, but I
accept your explanation.

Keep it as it is. TestCommon is due to be removed by the new
RunTests.gmk (which is properly integrated), and when that happens, we
can remove the exports then.

/Magnus

>
> Artem
>
>
> On 10/30/2017 10:50 AM, Magnus Ihse Bursie wrote:
>> On 2017-10-30 08:39, Artem Smotrakov wrote:
>>> cc'ing [hidden email] as David suggested.
>>>
>>> Artem
>>>
>>>
>>> On 10/27/2017 11:02 PM, Artem Smotrakov wrote:
>>>> Hello,
>>>>
>>>> Please review the following patch which adds support for
>>>> AddressSanitizer.
>>>>
>>>> AddressSanitizer is a runtime memory error detector which looks for
>>>> various memory corruption issues and leaks.
>>>>
>>>> Please refer to [1] for details. AddressSanitizer is available in
>>>> gcc 4.8+ and clang 3.1+
>>>>
>>>> The patch below introduces --enable-asan parameter for the
>>>> configure script which enables AddressSanitizer.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189800
>>>> Webrev: http://cr.openjdk.java.net/~asmotrak/8189800/webrev.00/
>> spec.gmk.in should only have export for variables that needs to be
>> exported in the environment for executing binaries, that is
>> ASAN_OPTIONS and LD_LIBRARY_PATH, not ASAN_ENABLED or DEVKIT_LIB_DIR.
>>
>> I'm also a bit curious about the addition of of DEVKIT_LIB_DIR. Would
>> you care to elaborate your thinking?
>>
>> Otherwise it looks good.
>>
>> /Magnus
>>
>>>>
>>>> [1] https://github.com/google/sanitizers/wiki/AddressSanitizer
>>>>
>>>> Artem
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR [10] 8189800: Add support for AddressSanitizer

Magnus Ihse Bursie
In reply to this post by David Holmes
On 2017-10-31 13:24, David Holmes wrote:
> Sounds reasonable. Anyone using older gcc simply won't/shouldn't
> enable Asan.

Agree. We will probably not be keeping any pretense of supporting
anything older than gcc 4.9 at some point in time anyway. I believe the
only known user of the oldest gcc is SAP for some of their platforms.

/Magnus

>
> Thanks,
> David
>
> On 31/10/2017 8:58 PM, Artem Smotrakov wrote:
>> Hi David,
>>
>> That's a good question, I thought about it. According to [1]:
>>
>> - recommended versions of gcc is 4.9.2
>> - the minimum accepted version of gcc is 4.7 (Older versions will
>> generate a warning by `configure` and are unlikely to work.)
>> - the minimum accepted version of clang is 3.2 (Older versions will
>> not be accepted by `configure`)
>>
>> It looks like that clang has to be at least 3.2 which should contain
>> AddressSanitizer. Only for gcc, there may be a chance that someone
>> wants to use 4.7. So, we might want to check version to see if it's
>> 4.7, although I am not sure how many people would like to use gcc
>> 4.7. As a result, this case didn't look very common to me, so I
>> preferred to simplify the patch, and didn't add such a check.
>>
>> Without version check, compilation is going to fail if gcc 4.7 is
>> used, and -fsanitize=address enabled.
>>
>> [1]
>> http://hg.openjdk.java.net/jdk10/master/file/438e0c9f2f17/doc/building.md
>>
>> Artem
>>
>> On 10/31/2017 01:37 PM, David Holmes wrote:
>>> Hi Artem,
>>>
>>> On 28/10/2017 6:02 AM, Artem Smotrakov wrote:
>>>> Hello,
>>>>
>>>> Please review the following patch which adds support for
>>>> AddressSanitizer.
>>>>
>>>> AddressSanitizer is a runtime memory error detector which looks for
>>>> various memory corruption issues and leaks.
>>>>
>>>> Please refer to [1] for details. AddressSanitizer is available in
>>>> gcc 4.8+ and clang 3.1+
>>>
>>> Should we be checking the version before adding the flags?
>>>
>>> Thanks,
>>> David
>>>
>>>> The patch below introduces --enable-asan parameter for the
>>>> configure script which enables AddressSanitizer.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8189800
>>>> Webrev: http://cr.openjdk.java.net/~asmotrak/8189800/webrev.00/
>>>>
>>>> [1] https://github.com/google/sanitizers/wiki/AddressSanitizer
>>>>
>>>> Artem
>>