RFR: 8333354: ubsan: frame.inline.hpp:91:25: and src/hotspot/share/runtime/frame.inline.hpp:88:29: runtime error: member call on null pointer of type 'const struct SmallRegisterMap' [v3]

[Kim Barrett]

On Wed, 24 Jul 2024 13:59:44 GMT, Matthias Baesken <mbaesken at openjdk.org> wrote:

>> When running with ubsan - enabled binaries, some tests trigger the following report :
>> 
>> src/hotspot/share/runtime/frame.inline.hpp:91:25: runtime error: member call on null pointer of type 'const struct SmallRegisterMap'
>>     #0 0x7fc1df86071e in unsigned char* frame::oopmapreg_to_location<SmallRegisterMap>(VMRegImpl*, SmallRegisterMap const*) const src/hotspot/share/runtime/frame.inline.hpp:91
>>     #1 0x7fc1df86071e in void OopMapDo<OopClosure, DerivedOopClosure, IncludeAllValues>::iterate_oops_do<SmallRegisterMap>(frame const*, SmallRegisterMap const*, ImmutableOopMap const*) src/hotspot/share/compiler/oopMap.inline.hpp:106
>>     #2 0x7fc1df8611df in void OopMapDo<OopClosure, DerivedOopClosure, IncludeAllValues>::oops_do<SmallRegisterMap>(frame const*, SmallRegisterMap const*, ImmutableOopMap const*) src/hotspot/share/compiler/oopMap.inline.hpp:157
>>     #3 0x7fc1df8611df in FrameOopIterator<SmallRegisterMap>::oops_do(OopClosure*) src/hotspot/share/oops/stackChunkOop.cpp:63
>>     #4 0x7fc1dcfc8745 in BarrierSetStackChunk::encode_gc_mode(stackChunkOopDesc*, OopIterator*) src/hotspot/share/gc/shared/barrierSetStackChunk.cpp:85
>>     #5 0x7fc1df854080 in bool TransformStackChunkClosure::do_frame<(ChunkFrames)0, SmallRegisterMap>(StackChunkFrameStream<(ChunkFrames)0> const&, SmallRegisterMap const*) src/hotspot/share/oops/stackChunkOop.cpp:319
>>     #6 0x7fc1df854080 in void stackChunkOopDesc::iterate_stack<(ChunkFrames)0, TransformStackChunkClosure>(TransformStackChunkClosure*) src/hotspot/share/oops/stackChunkOop.inline.hpp:233
>>     #7 0x7fc1df82f184 in void stackChunkOopDesc::iterate_stack<TransformStackChunkClosure>(TransformStackChunkClosure*) src/hotspot/share/oops/stackChunkOop.inline.hpp:199
>> 
>> Seems in case of (at least) class SmallRegisterMap we miss handling nullptr .
>
> Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision:
> 
>   ATTRIBUTE_NO_UBSAN must be after template typename ...

Changes requested by kbarrett (Reviewer).

> I think this is intended. No instances of SmallRegisterMap are ever created.
> 
> Instead [SmallRegisterMap::instance](https://github.com/openjdk/jdk/blob/5b4824cf9aba297fa6873ebdadc0e9545647e90d/src/hotspot/cpu/x86/smallRegisterMap_x86.inline.hpp#L34C20-L34C36) is used:
> 
> ```c++
>   static constexpr SmallRegisterMap* instance = nullptr;
> ```
> 
> The type is the only information that is actually used.

Being intentional doesn't make it any less invalid.

Here's an untested change that I think will fix the problem.
https://github.com/openjdk/jdk/compare/master...kimbarrett:openjdk-jdk:smallregmap?expand=1

src/hotspot/share/runtime/frame.inline.hpp line 86:

> 84: 
> 85: template <typename RegisterMapT>
> 86: ATTRIBUTE_NO_UBSAN

That's not good enough.  Turning off the ubsan warning doesn't prevent the compiler from doing
unexpected and potentially bad things with invalid code.

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

PR Review: https://git.openjdk.org/jdk/pull/20296#pullrequestreview-2197514292
PR Comment: https://git.openjdk.org/jdk/pull/20296#issuecomment-2248704435
PR Comment: https://git.openjdk.org/jdk/pull/20296#issuecomment-2248706749
PR Review Comment: https://git.openjdk.org/jdk/pull/20296#discussion_r1690277354