RFR(S) 8209950: SIGBUS in CodeHeapState::print_names()

[Schmidt, Lutz]

Hi Vladimir,

how about this one? http://cr.openjdk.java.net/~lucy/webrevs/8209950.01/
I have factored out the complex if as suggested. 

@Tobias: can you live with this version as well?

Thanks, Lutz

On 28.08.18, 23:29, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:

    On 8/28/18 1:58 PM, Schmidt, Lutz wrote:
    > Hi Vladimir,
    > 
    > I'm not sure if I understand your proposal correctly. So I try to rephrase it according to my understanding. You can then please let me know if I got it right. Here we go:
    > 
    > You suggest to factor out the contents of the complex if statement into a separate function (like static bool nmethod_access_is_safe() or similar). The function could live in class CodeCache, class CompiledMethod, or somewhere similar. What is your favorite place?
    > 
    > If this is your request, I am absolutely in favor of it. If not: please correct me.
    
    Yes, it is exactly my request. I would put it into CompiledMethod.
    
    > 
    > Re performance:
    > 
    > Yes, both code locations are used for tracing/debugging/analysis purposes only. The do not impact production workloads. But: I have seen "safepoint timeouts" in our tests with -XX:+CountCompiledCalls. Just imagine how many (compiled) methods exist after a long run of a large workload with a big code heap. You certainly don't want your trace data to be truncated after investing so much patience and machine time. What I want to say: It's worthwhile considering performance even for debugging code. Not to stringently, but spend a thought.
    > 
    > CodeHeap state analytics can even be requested in production systems. That's what we at SAP originally designed them for, strongly influenced by our support engineers.
    Got it.
    
    Thanks,
    Vladimir
    
    > 
    > OK, I come to an end now. Sorry for the long text.
    > 
    > Thanks,
    > Lutz
    > 
    > On 28.08.18, 21:51, "Vladimir Kozlov" <vladimir.kozlov at oracle.com> wrote:
    > 
    >      Hi Lutz,
    >      
    >      Thank you for looking on this. Changes are fine but I think you introduce a new function to fold
    >      these set of checks since they become so similar.  I am not shere your concern about
    >      os::is_readable_pointer() expense. Both places you fixed are not performance critical.
    >      
    >      Thanks,
    >      Vladimir
    >      
    >      On 8/27/18 5:30 AM, Schmidt, Lutz wrote:
    >      > Dear all,
    >      >
    >      > may I please request reviews for my change? It hardens code which iterates over all nmethods in the code cache, e.g. by CodeCache::nmethods_do(). The bug refers to an issue in share/code/codeHeapState.cpp, but the same code exists in share/runtime/sharedRuntime.cpp. SIGBUS/SIGSEGV errors have been observed recently for both locations. Holding the CodeCache_lock while iterating, as asserted by CodeCache::nmethods_do(), does not help.
    >      >
    >      > The solution is to check yet another pointer for NULL and accessibility before actually using it. Not-NULL but invalid pointers have been observed. That makes accessibility checks necessary. The calls to os::is_readable_pointer() are potentially expensive (if they fail), but the alternative would be a SIGBUS/SIGSEGV.
    >      >
    >      > This fix extends JDK-8209588 for the sharedRuntime.cpp part. As noted there, it may have impact as far back as jdk9 (for sharedRuntime.cpp).
    >      >
    >      > Bug:    https://bugs.openjdk.java.net/browse/JDK-8209950
    >      > Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8209950.00/
    >      >
    >      > Thank you,
    >      > Lutz
    >      >
    >      >
    >      
    >