Bug ID: JDK-6456897 AsyncGetCallTrace() can observe abstract methods

Type: Bug
Component: hotspot
Sub-Component: svc
Affected Version: 6

Priority: P4
Status: Open
Resolution: Unresolved
OS: generic
CPU: generic

Submitted: 2006-08-03
Updated: 2019-02-04

Other
tbdUnresolved

I ran into an assertion failure when stress testing the fix for the following bug:

    6456834 3/3 part of AsyncGetCallTrace fix for 6379830 in needed on
                Solaris SPARC

Here is the assertion failure message:

#  Internal Error (src/share/vm/oops/methodOop.cpp, 181 [ Patched ]), pid=17493, tid=2
#
# Java VM: Java HotSpot(TM) Client VM (mustang-fastdebug-debug mixed mode)
#
# Error: assert((is_native() && bci == 0) || (!is_native() && 0 <= bci && bci <
code_size()),"illegal bci")

Not a priority for 11, moving it to 12.
18-01-2018
Should be looked at in 11 if we are going to allocate resources for AsyncGetCallTrace(); definitely don't need this for 10.
05-04-2017
EVALUATION Steve G. had some concerns about just how we got our hands on an abstract method. I did a detailed analysis of my last fastdebug crash and attached that analysis as threads.log.2291. I figured out that we got the abstract method from the frame that we built from the signal handler's ucontext. The frame passed a bunch of existing sanity checks: - forte_safe_for_sender(fr, thread) - !fr->is_first_frame() - fr->is_interpreted_frame() - fr->is_interpreted_frame_valid() - fr->fp() != NULL - forte_is_valid_method(method) There are no obvious existing sanity checks that would make this failure mode go away. The fact that the frame passed a forte_safe_for_sender() call when the debugger sees an invalid sender frame was surprising. However, that could just be brokeness in the debugger. John Rose's suggestion to modify validate_bci_from_bcx() which is called from forte_is_walkable_interpreted_frame() seems like the best solution.
21-08-2006
EVALUATION John R. sent the following via e-mail: I think you need one more sanity check (but surely not the last) on the methodOop, to make sure it is not an abstract method. I suggest checking the access_flags for the known sane patterns. I think you ought to do this in methodOopDesc::validate_bci_from_bcx: if (is_abstract()) { bci = -1; // an abstract method, or some other garbage } else if (bcx == 0 \|\| (address)bcx == code_base()) { // code_size() may return 0 and we allow 0 here // the method may be native bci = 0; ... That function is supposed to perform validation of this sort of thing.
04-08-2006
SUGGESTED FIX Here is the context diff for the proposed fix: ------- src/share/vm/oops/methodOop.cpp ------- * /tmp/sccs.0vaWQQ Thu Aug 3 12:11:19 2006 --- methodOop.cpp Thu Aug 3 12:11:13 2006 *********** * 178,186 **** } address methodOopDesc::bcp_from(int bci) const { ! assert((is_native() && bci == 0) \|\| (!is_native() && 0 <= bci && bci < code_size()), "illegal bci"); address bcp = code_base() + bci; ! assert(is_native() && bcp == code_base() \|\| contains(bcp), "bcp doesn't belong to this method"); return bcp; } --- 178,194 ---- } address methodOopDesc::bcp_from(int bci) const { ! // bci of zero (0) is always okay and maps to bcp == code_base(). For ! // non-native methods, bci must be in the method's code range. Since ! // bci == 0 is checked in the first part, we could use '0 < bci' in ! // the second part, but we don't want to confuse anyone looking at ! // just the range check. ! assert(bci == 0 \|\| (!is_native() && 0 <= bci && bci < code_size()), ! "illegal bci"); address bcp = code_base() + bci; ! // bcp == code_base() is always okay ! assert(bcp == code_base() \|\| contains(bcp), ! "bcp doesn't belong to this method"); return bcp; } Update: The above suggested fix was discussed on the hs-runtime alias and there was pretty much universal agreement that the above changes are not needed.
03-08-2006
EVALUATION Here is the code in question: address methodOopDesc::bcp_from(int bci) const { assert((is_native() && bci == 0) \|\| (!is_native() && 0 <= bci && bci < code_size()), "illegal bci"); address bcp = code_base() + bci; assert(is_native() && bcp == code_base() \|\| contains(bcp), "bcp doesn't belong to this method"); return bcp; } If the bci parameter comes in as zero, this code is okay as long as the method is native. However, I've observed an interpreted method where code_size() is zero. Granted this was with AsyncGetCallTrace(), but a sighting is a sighting. Both of the assertions above need to be tweaked. For the first: // bci of zero (0) is always okay and maps to bcp == code_base(). For non-native // methods, bci must be in the method's code range. Since bci == 0 is checked in // the first part, we could use '0 < bci' in the second part, but we don't want // to confuse anyone looking at the range check. assert(bci == 0 \|\| (!is_native() && 0 <= bci && bci < code_size()), "illegal bci"); If code_size() == 0, then 'bci < code_size()' won't return true because there is no code at bci == 0. This is why 'bci == 0' has to be handled specially. For the second: // bcp == code_base() is always okay assert(bcp == code_base() \|\| contains(bcp), "bcp doesn't belong to this method"); If code_size() == 0, then contains(bcp) won't return true because there is no code at address bcp. This is why 'bcp == code_base()' has to be handled specially. Update: The above evaluation and code snippets were discussed on the hs-runtime alias and there is pretty much universal agreement that I'm heading down the wrong path here. More investigation is needed.
03-08-2006