RFR: 8340491: Thread stack-base assertion should report which thread has the un-set stack

[David Holmes]

On Fri, 20 Sep 2024 05:07:43 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Please review this simple enhancement to an assertion so we can identify which thread had the problem.
>> 
>> I had to move the function to the cpp file due to include file issues.
>> 
>> We are limited in what we can print due to this being used very early in the thread initialization process - in particular no ResourceMarks are possible so we can't print the name.
>> 
>> Testing:
>>  - tier 5 (used to debug [JDK-8340401](https://bugs.openjdk.org/browse/JDK-8340401))
>>  - tiers 1-3 sanity
>> 
>> Thanks
>
> Looks fine, but could you explain when this is needed?
> 
> If it is the current thread that is crashes then we already print the thread info.
> 
> If it is crashing when called on another thread, it might be easier to add informative asserts in that sub-system instead. You could for example add a non-asserting getter and perform all relevant checks on the caller side.
> 
> Another alternative could be to leverage the VMErrorCallback functionality I introduced a little while ago. With that you can register a callback to be called by the hs_err printer if the current thread crashes. With that you can have much more context about what the calling code was doing and what information that thread has available. That would look something like this (~I've not compiled this, so there might be errors~ Edit: updated the code):
> 
> 
> diff --git a/src/hotspot/share/nmt/memMapPrinter.cpp b/src/hotspot/share/nmt/memMapPrinter.cpp
> index 5f920b102a9..a2aae39b0ca 100644
> --- a/src/hotspot/share/nmt/memMapPrinter.cpp
> +++ b/src/hotspot/share/nmt/memMapPrinter.cpp
> @@ -44,6 +44,7 @@
>  #include "runtime/vmThread.hpp"
>  #include "utilities/globalDefinitions.hpp"
>  #include "utilities/ostream.hpp"
> +#include "utilities/vmError.hpp"
> 
>  // Note: throughout this code we will use the term "VMA" for OS system level memory mapping
> 
> @@ -164,8 +165,29 @@ class CachedNMTInformation : public VirtualMemoryWalker {
> 
>  /////// Thread information //////////////////////////
> 
> +class VMAInspectionErrorCallback : public VMErrorCallback {
> +  const void* const   _from;
> +  const void* const   _to;
> +  const Thread* const _thread;
> +
> +public:
> +  VMAInspectionErrorCallback(const void* from, const void* to, const Thread* thread) :
> +    _from(from), _to(to), _thread(thread) {}
> +
> +  void call(outputStream* st) override {
> +    st->print_cr("Crashing while printing VMA details for " PTR_FORMAT " " PTR_FORMAT " for thread %s with id: %d",
> +                 p2i(_from),
> +                 p2i(_to),
> +                 _thread->name(),
> +                 _thread->osthread() != nullptr ? _thread->osthread()->thread_id() : 0);
> +  }
> +};
> +
>  // Given a VMA [from, to) and a thread, check if vma intersects with thread stack
>  static bool vma_touches_thread_stack(const void* from, const void* to, const Thread* t) {
> +  VMAInspectionErrorCallback on_error(from, to, t);
> +  VMErrorCallbackMark mark(&on_error);
> +
>    // Java thread stacks (and sometimes also other threads) have guard pages. Therefore they typically occupy
>    // at least two distinct neig...

Thanks for the review @stefank !

> If it is crashing when called on another thread, it might be easier to add informative asserts in that sub-system instead. You could for example add a non-asserting getter and perform all relevant checks on the caller side.

The assert is firing when `stack_base()` is called on another thread. In that case we had nothing to tell us which thread it was, hence this enhancement to provide that information. We want this asserting getter to be used, and for the assert to fire .

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

PR Comment: https://git.openjdk.org/jdk/pull/21102#issuecomment-2362891578