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

[Stefan Karlsson]

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

>> David Holmes has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Need ifdef ASSERT
>
> 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.

That was fully understood and I think you are missing my point. I was trying to convey that it might be better to rewrite the calling code to perform its own checks / logging instead of relying on a "low-level" assert in Thread. 2c.

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

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