RFR (S) 8217879: hs_err should print more instructions in hex dump

[Stefan Karlsson]

Hi Aleksey,

I like this proposal!

I looked at the patch and have some comments:

https://cr.openjdk.java.net/~shade/8217879/webrev.02/src/hotspot/share/runtime/os.cpp.udiff.html

------------------------------------------------------------------------------
Did you intend to use & instead of && here ?:

+ while (delta > 0 & pc > low && !is_readable_pointer(low)) {

------------------------------------------------------------------------------Regarding this comment:

+ // Safe probing relies on the conjecture that the entire pages are 
either + // readable or unreadable. Therefore, we cannot have delta 
larger than page + // size, since otherwise we can discover "far" page 
as readable, while "near" + // page is not readable.

The code doesn't seem to prevent this. If 'pc' is somewhere at the 
beginning of a page, then

'low = pc - delta' could cross over to the previous page.
------------------------------------------------------------------------------ 
The probing of safe reads only happen at power-of-two offsets from 'pc'. 
Did you consider using binary search to find the exact break point where 
the reading fails? It's probably overkill for this feature though.
------------------------------------------------------------------------------ 
I see that: bool os::is_readable_range(const void* from, const void* to) 
{ for (address p = align_down((address)from, min_page_size()); p < to; p 
+= min_page_size()) { if (!is_readable_pointer(p)) { return false; } } 
return true; } is using min_page_size(), instead of os::vm_page_size(), 
which has this comment: // Return a lower bound for page sizes. Also 
works before os::init completed. static size_t min_page_size() { return 
4 * K; } Is this something this code needs to consider as well? 
os::vm_page_size() can assert / return -1 if called too early. Thanks, 
StefanK

On 2019-01-28 17:48, Aleksey Shipilev wrote:
> RFE:
>    https://bugs.openjdk.java.net/browse/JDK-8217879
>
> Fix:
>    http://cr.openjdk.java.net/~shade/8217879/webrev.01/
>
> "Instructions" block is useful when following up on hs_errs that happened without the disassembler
> attached, which is usually the case coming from users. One can use the disassembler [1] to look
> around the code that was crashing, and get extended conjectures why the error happened, including
> rewinding a bit of history. However, current window is sometimes too small to infer enough context.
> I propose we extend it!
>
> The patch also commons the paths across OS/Arch-specific files to that current "delta" appears less
> of the magic number. Plus, it adds cr()-s for consistency across the arches and within the methods.
>
> Testing: eyeballing hs_errs from artificial crashes, Linux x86_64 build, jdk-submit
>
> Thanks,
> -Aleksey
>
> [1] I use https://onlinedisassembler.com, for example.
>