RFR: JDK-8298908: Instrument Metaspace for ASan [v12]

[Magnus Ihse Bursie]

On Tue, 10 Jan 2023 23:17:06 GMT, Justin King <jcking at openjdk.org> wrote:

>> This change instruments Metaspace for ASan. Metaspace allocates memory using `mmap`/`munmap` which ASan is not aware of. Fortunately ASan supports applications [manually poisoning/unpoisoning memory](https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning). ASan is able to detect poisoned memory, similar to `use-after-free`, and will raise an error similarly called `use-after-poison`. This provides and extra layer of defense and confidence.
>> 
>> The header `sanitizers/address.h` defines macros for poisoning/unpoisoning memory regions. These macros can be used regardless of build mode. When ASan is not available, they are implemented using a NOOP approach which still compiles the arguments but does so such that they will be stripped out by the compiler due to being unreachable. This helps with maintenance.
>> 
>> This also has the added benefit of making [LSan](https://bugs.openjdk.org/browse/JDK-8298445) more accurate and deterministic, as LSan will not look for pointers to malloc memory in poisoned memory regions.
>> 
>> IMO the benefit of doing this greatly outweighs the cost.
>
> Justin King has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update sanitizers/address.h based on review
>   
>   Signed-off-by: Justin King <jcking at google.com>

Build changes are trivially fine. The rest of the PR can be resolved without the build team's involvement.

The build changes were trivial. You are correct that adding the build label is the appropriate thing to do, so the PR comes to the build team's attention. However, if the changes is more "incidentally" in the build system (as here, were the added flags do not really change any build behavior), then there is really nothing to say from a build perspective.

I handle such PRs in one of two ways, either I approve them saying "build changes are trivially fine". Or, in cases like this, when the *actual* change is apparently controversial, and triggers a long debate, I try to save build-dev from a lengthy and irrelevant discussion by removing the label. But sure, I should probably add a build approval as well even in those cases.

-------------

Marked as reviewed by ihse (Reviewer).

PR: https://git.openjdk.org/jdk/pull/11702