On Mon, 9 Jan 2023 07:39:03 GMT, Thomas Stuefe <[email protected]> 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 <[email protected]> > > 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
