[External] : Re: Crash due to bad oop map

[dean.long at oracle.com]

On 7/21/22 10:57 PM, Denghui Dong wrote:
> Hi @dean-long,
> 
> I tried, and it works.
> 
> Another way is to skip the processing of MachTemp when building the oop 
> map. Do you think this is the correct way?

That might not be the best fix.  It sounds like a special case to skip 
MachTemp for oop maps.  The fact that the MachTemp gets scheduled into a 
different block seems like the real problem.

dl

> Denghui Dong
> 
>     ------------------------------------------------------------------
>     From:dean.long <dean.long at oracle.com>
>     Send Time:2022年7月22日(星期五) 11:28
>     To:undefined <undefined>; undefined <undefined>
>     Subject:Re: Crash due to bad oop map
> 
>     Hi Denghui Dong.  This looks like JDK-8051805
>     (https://bugs.openjdk.org/browse/JDK-8051805).  As you discovered,
>     it is
>     triggered when RegN is used as a TEMP.  As a work-around, have you
>     tried
>     using rRegI instead of rRegN for your TEMPs?
> 
>     dl
> 
>     On 7/21/22 12:11 AM, Denghui Dong wrote:
>      > Hi team,
>      >
>      > We encountered a crash due to a bad oop map.
>      > The following steps can quickly reproduce the problem on JDK
>     master on
>      > Linux x64:
>      > 1. Modify z_x86_64.ad and add 3 temporary registers of type rRegN to
>      > zLoadP(If this change is illegal, please let me know)
>      > ```
>      > diff --git a/src/hotspot/cpu/x86/gc/z/z_x86_64.ad
>      > b/src/hotspot/cpu/x86/gc/z/z_x86_64.ad
>      > index f3e19b41733..129c93d3da8 100644
>      > --- a/src/hotspot/cpu/x86/gc/z/z_x86_64.ad
>      > +++ b/src/hotspot/cpu/x86/gc/z/z_x86_64.ad
>      > @@ -63,11 +63,11 @@ static void
>     z_load_barrier_cmpxchg(MacroAssembler&
>      > _masm, const MachNode* node,
>      > %}
>      > // Load Pointer
>      > -instruct zLoadP(rRegP dst, memory mem, rFlagsReg cr)
>      > +instruct zLoadP(rRegP dst, memory mem, rRegN tmp1, rRegN tmp2,
>     rRegN
>      > tmp3, rFlagsReg cr)
>      > %{
>      >    predicate(UseZGC && n->as_Load()->barrier_data() != 0);
>      >    match(Set dst (LoadP mem));
>      > -  effect(KILL cr, TEMP dst);
>      > +  effect(KILL cr, TEMP dst, TEMP tmp1, TEMP tmp2, TEMP tmp3);
>      >    ins_cost(125);
>      > ```
>      > 2. Run the following code
>      > (./build/linux-x86_64-server-fastdebug/images/jdk/bin/java
>     -XX:-Inline
>      > -XX:CompileCommand=compileonly,Test::call -XX:+PrintAssembly
>      > -XX:PrintIdealGraphLevel=2 -XX:PrintIdealGraphFile=call.xml
>     -XX:+UseZGC
>      > -XX:+UseNewCode -XX:+PrintGC -Xmx500m -Xms500m -XX:-Inline Test)
>      > ```
>      > import java.util.concurrent.locks.Lock;
>      > import java.util.concurrent.locks.ReentrantLock;
>      > class Test {
>      >    private Lock lock = new ReentrantLock();
>      >    public static void main(String... args) throws Exception {
>      >      new Thread(() -> {
>      >        while (true) {
>      >          byte[] b = new byte[10 * 1024 * 1024];
>      >        }
>      >      }).start();
>      >      while (true) {
>      >        new Test().call(() -> { new Object(); });
>      >      }
>      >    }
>      >    public void call(Runnable cb) {
>      >      lock.lock();
>      >      try {
>      >        cb.run();
>      >      } finally {
>      >        lock.unlock();
>      >      }
>      >    }
>      > }
>      > ```
>      >
>      > It can be observed through the IGV Final Code that zLoadP(202,
>     204) and
>      > a MachTemp(81) it uses are placed in different blocks. When
>     processing
>      > CallStaticJavaDirect(74) in the c2 buildOopMap step, MachTemp(81) is
>      > considered to be alive. Because the register type specified by
>     the above
>      > modification is rRegN (if it is specified as rRegI or rRegP, no
>     crash
>      > will occur), so the type of MachTemp(81) is narrowOop, which will
>      > eventually be added to the oopMap of CallStaticJavaDirect(74). But
>      > logically, zLoadP doesn't depend on the original value of
>     MachTemp(81),
>      > so I don't think it should be added to oopMap.
>      >
>      > I put the ideal graph file to
>     http://cr.openjdk.java.net/~ddong/call.xml
>      > <http://cr.openjdk.java.net/~ddong/call.xml>
>      >
>      > I think the problem may lie in two places:
>      > 1. gcm and regAlloc do not put MachTemp(81) in the correct location
>      > 2. buildOopMap should not treat MachTemp as Oop because its original
>      > value is meaningless
>      >
>      > I'm not a JIT expert and sorry if there is anything unnatural or
>      > non-standard in the above description.
>      >
>      > Any input is appreciated.
>      >
>      > Denghui Dong