On Mon, 9 Jan 2023 07:39:03 GMT, Thomas Stuefe <stu...@openjdk.org> wrote:
>> Justin King has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Use -fno-common as suggested by ASan docs
>>
>> Signed-off-by: Justin King <jck...@google.com>
>
> src/hotspot/share/memory/metaspace/chunkManager.cpp line 137:
>
>> 135: // - Or, if the necessary space cannot be committed because we hit a
>> commit limit.
>> 136: // This may be either the GC threshold or MaxMetaspaceSize.
>> 137: Metachunk* ChunkManager::get_chunk_locked(chunklevel_t preferred_level,
>> chunklevel_t max_level, size_t min_committed_words) {
>
> Please add `assert_lock_strong(Metaspace_lock);`
Done.
> src/hotspot/share/memory/metaspace/chunkManager.cpp line 247:
>
>> 245: // this function returns !!
>> 246: void ChunkManager::return_chunk(Metachunk* c) {
>> 247: ASAN_POISON_MEMORY_REGION(c->base(), c->word_size() * BytesPerWord);
>
> Please add comment:
>
> // It is valid to poison the chunk payload area at this point since its
> physically separated from the chunk meta info.
Done.
> src/hotspot/share/memory/metaspace/virtualSpaceNode.cpp line 239:
>
>> 237: assert_is_aligned(_word_size, chunklevel::MAX_CHUNK_WORD_SIZE);
>> 238:
>> 239: // Poison the memory region. It will be unpoisoned later by
>> MetaspaceArena.
>
> Can you be a bit more specific? Proposal:
> "It will be unpoisened on a per-chunk base for chunks that are handed to
> arenas."
Done.
> src/hotspot/share/runtime/os.cpp line 951:
>
>> 949: NO_SANITIZE_ADDRESS void os::print_hex_dump(outputStream* st, address
>> start, address end,
>> 950: int unitsize, int
>> bytes_per_line,
>> 951: address logical_start) {
>
> Out of scope for this bug; can this be moved to a different RFE?
>
> While looking at this, I found that os::print_hex_dump has a little error. It
> is not supposed to access memory directly but only via SafeFetch (in order to
> print '?' for unreadable locations). SafeFetch would be the natural point for
> asan exclusion. I filed a bug for this:
> https://bugs.openjdk.org/browse/JDK-8299790
Sure, reverted the changes to this file. I think SafeFetch is fine already as
its assembly and instrumentation won't happen, but we can double check later
once its fixed. I think there are some non-assembly versions of SafeFetch and
that would need to be fixed.
-------------
PR: https://git.openjdk.org/jdk/pull/11702
