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

Investigate use of Thread::stack_base() and queries for "in stack"

    XMLWordPrintable

    Details

    • Subcomponent:
    • Resolved In Build:
      b11

      Description

      In light of JDK-8215355 we should check all uses of Thread::stack_base() to watch for range tests that should be exclusive but are inclusive, and vice-versa.

      Review comments from JDK-8215355

      On 18/11/2019 9:58 pm, Thomas Stüfe wrote:
      > There might be more cases like this, e.g.
      >
      > frame_x86.cpp frame::is_interpreted_frame_valid():
      >
      > if (locals > thread->stack_base() || locals < (address) fp()) return false;
      >
      > Many of them could use Thread::in_usable_stack(), I assume.

      On 19/11/2019 3:36 am, Daniel D. Daugherty wrote:
      > src/hotspot/share/runtime/thread.cpp
      >
      > Not your issue, but these two lines feel strange/wrong:
      >
      > L1008: // Allow non Java threads to call this without stack_base
      > L1009: if (_stack_base == NULL) return true;
      >
      > When _stack_base is NULL, any 'adr' is in the caller's stack? The
      > comment is not helping understand why this is so...
      >
      > src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/JavaThread.java
      >
      > Again, not your issue, but these four lines are questionable:
      >
      > L383 Address sp = lastSPDbg();
      > L384 Address stackBase = getStackBase();
      > L385 // Be robust
      > L386 if (sp == null) return false;
      >
      > I can see why a NULL sp would cause a "false" return since obviously
      > something is a amiss in the frame. However, the C++ code doesn't make
      > this check so why does the SA code?
      >
      > And this code doesn't check stackBase == NULL so it's not matching
      > the C++ code either.

      > So we have Thread::is_in_usable_stack(), Thread::on_local_stack() and
      > Thread::is_in_stack()? I haven't compared all three side by side, but
      > there might be some cleanup work that can be done here (in a different
      > bug).

        Attachments

          Issue Links

            Activity

              People

              Assignee:
              dholmes David Holmes
              Reporter:
              dholmes David Holmes
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved: