Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8227338

templateInterpreter.cpp: copy_table() needs to be safer

    Details

    • Subcomponent:
    • Resolved In Build:
      b06

      Backports

        Description

        This issue came up during the code review for the following fix:

            JDK-8227117 normal interpreter table is not restored after
                                   single stepping with TLH

        The issue is not caused by TLH or by JDK-8227117, but dates
        back to the earliest days of HotSpot.

        Here is Erik's code review comment and my reply:

        On 7/5/19 1:07 PM, Daniel D. Daugherty wrote:
        > On 7/4/19 3:10 AM, Erik Österlund wrote:
        >> Hi Dan,
        >>
        >> Thanks for picking this up. The change looks good.
        >
        > Thanks! Of course, just the size of the comment below makes me wonder
        > what I got myself into... :-) And I was so happy that the non-logging
        > part of the fix was an else-statement with _one_ line...
        >
        >
        >> However, when reviewing this, I looked at the code for actually restoring the table (ignore/notice safepoints). It copies the dispatch table for the interpreter. There is a comment stating it is important the copying is atomic for MT-safety, and I can definitely see why. However, the copying the line after that comment is in fact not atomic.
        >
        > Actually, the comment doesn't mention 'atomic', but that's probably
        > because the code and the comment are very, very old. It mentions
        > 'word wise for MT safety' and I agree that 'atomic' is what the
        > person likely meant...
        >
        > The history:
        >
        > $ sgv src/share/vm/interpreter/templateInterpreter.cpp | grep 'The copy has to occur word wise for MT safety'
        > 1.1 // Copy non-overlapping tables. The copy has to occur word wise for MT safety.
        >
        > $ sp -r1.1 src/share/vm/interpreter/templateInterpreter.cpp
        > src/share/vm/interpreter/SCCS/s.templateInterpreter.cpp:
        >
        > D 1.1 07/08/29 13:42:26 sgoldman 1 0 00600/00000/00000
        > MRs:
        > COMMENTS:
        > 6571248 - continuation_for is specialized for template interpreter
        >
        > Hmmm... I expected that comment to be even older... ahhhh... a little
        > more poking around and I found:
        >
        > $ sgv -r1.147 src/share/vm/interpreter/interpreter.cpp | grep 'The copy has to occur word wise for MT safety'
        > 1.147 // Copy non-overlapping tables. The copy has to occur word wise for MT safety.
        >
        > $ sp -r1.147 src/share/vm/interpreter/interpreter.cpp
        > src/share/vm/interpreter/SCCS/s.interpreter.cpp:
        >
        > D 1.147 99/02/17 10:14:36 steffen 235 233 00008/00002/00762
        > MRs:
        > COMMENTS:
        >
        > This makes more sense (timeline wise) and dates back to when all
        > of the interpreter was in vm/interpreter/interpreter.cpp.
        >
        >
        >> Here is the copying code in templateInterpreter.cpp:
        >>
        >> static inline void copy_table(address* from, address* to, int size) {
        >> // Copy non-overlapping tables. The copy has to occur word wise for MT safety.
        >> while (size-- > 0) *to++ = *from++;
        >> }
        >>
        >> Copying using a loop of non-volatile loads and stores can and definitely will on some compilers turn into memcpy calls instead as the compiler (correctly) considers that an equivalent transformation.
        >
        > Yet another C++ compiler optimization land mine... sigh...
        >
        >
        >> And memcpy does not guarantee atomicity. Indeed on some platforms it is not atomic. On some platforms it will even enjoy out-of-thin-air values.
        >
        > That last bit is scary...
        >
        >
        >> Perhaps Copy::disjoint_words_atomic() would be a better choice for atomic word copying. If not, at the very least we should use Atomic::load/store here.
        >
        > Copy::disjoint_words_atomic() sounds appealing...
        >
        > For those folks that aren't familiar with this part of safepointing...
        >
        > SafepointSynchronize::arm_safepoint() calls Interpreter::notice_safepoints()
        > which calls calls copy_table(). So we're not at a safepoint yet, and, in fact,
        > we're trying to bring those pesky JavaThreads to a safepoint...
        >
        > SafepointSynchronize::disarm_safepoint() calls Interpreter::ignore_safepoints()
        > which also calls copy_table(). However, we did that before we have woken the
        > JavaThreads that are blocked for the safepoint so that use of copy_table is safe:
        >
        >
        > // Release threads lock, so threads can be created/destroyed again.
        > Threads_lock->unlock();
        >
        > // Wake threads after local state is correctly set.
        > _wait_barrier->disarm();
        > }
        >
        > The 'Threads_lock->unlock()' should synchronize memory so that the restored
        > table should be properly synced out to memory...
        >
        >
        >> Having said that, the fix for that issue seems like a separate RFE, because it has been sitting there for a lot longer than TLH has been around.
        >
        > Yes I would like to keep the copy_table() issue for a separate bug (not RFE).
        > I'll file a follow up bug after the dust settles for 8227117.
        >
        > Thanks again for the review!
        >
        > Dan
        >
        >>
        >> Thanks,
        >> /Erik

          Attachments

            Issue Links

              Activity

                People

                • Assignee:
                  dcubed Daniel Daugherty
                  Reporter:
                  dcubed Daniel Daugherty
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  4 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved: