RFR(L): 8180032: Unaligned pointer dereference in ClassFileParser (original) (raw)

Robbin Ehn robbin.ehn at oracle.com
Sat May 27 11:53:08 UTC 2017

Hi Mikael,

I see you have pushed this, good! Sorry for the late response.

On 2017-05-26 00:00, Mikael Vidstedt wrote:

Reasonable?

+1, thanks!

/Robbin

Cheers, Mikael [1] http://cr.openjdk.java.net/~mikael/webrevs/8180032/webrev.01/hotspot/webrev/

On May 18, 2017, at 5:18 PM, David Holmes <david.holmes at oracle.com_ _<mailto:david.holmes at oracle.com>> wrote:

On 19/05/2017 9:19 AM, Mikael Vidstedt wrote:

On May 18, 2017, at 3:50 PM, David Holmes <david.holmes at oracle.com_ _<mailto:david.holmes at oracle.com> <mailto:david.holmes at oracle.com>> wrote:

Hi Mikael, On 19/05/2017 8:15 AM, Mikael Vidstedt wrote:

On May 18, 2017, at 2:59 AM, Robbin Ehn <robbin.ehn at oracle.com_ _<mailto:robbin.ehn at oracle.com> <mailto:robbin.ehn at oracle.com>> wrote:

Hi, On 05/17/2017 03:46 AM, Kim Barrett wrote: On May 9, 2017, at 6:40 PM, Mikael Vidstedt <mikael.vidstedt at oracle.com <mailto:mikael.vidstedt at oracle.com> <mailto:mikael.vidstedt at oracle.com>> wrote:

Warning: It may be wise to stock up on coffee or tea before reading this. Bug: https://bugs.openjdk.java.net/browse/JDK-8180032 Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8180032/webrev.00/hotspot/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8180032/webrev.00/hotspot/webrev/> Not a review, just a question. ------------------------------------------------------------------------------ src/cpu/x86/vm/bytesx86.hpp 40 template 41 static inline T getnative(const void* p) { 42 assert(p != NULL, "null pointer"); 43 44 T x; 45 46 if (isptraligned(p, sizeof(T))) { 47 x = (T)p; 48 } else { 49 memcpy(&x, p, sizeof(T)); 50 } 51 52 return x; I'm looking at this and wondering if there's a good reason to not just unconditionally use memcpy here. gcc -O will generate a single move instruction for that on x8664. I'm not sure what happens on 32bit with an 8 byte value, but I suspect it will do something similarly sensible, e.g. 2 4 byte memory to memory transfers. Unconditionally memcpy would be nice! Are going to look into that Mikael? It’s complicated… We may be able to switch, but there is (maybe) a subtle reason why the alignment check is in there: to avoid word tearing.. Think of two threads racing: * thread 1 is writing to the memory location X * thread 2 is reading from the same memory location X Will thread 2 always see a consistent value (either the original value or the fully updated value)? We're talking about internal VM load and stores rights? For those we need to use appropriate atomic routine if there are potential races. But we should never be mixing these kind of accesses with Java level field accesses - that would be very broken. That seems reasonable, but for my untrained eye it’s not trivially true that relaxing the implementation is correct for all the uses of the get/put primitives. I am therefore a bit reluctant to do so without understanding the implications. If a Copy routine doesn't have Atomic in its name then I don't expect atomicity. Even then unaligned accesses are not atomic even in the Atomic routine! But I'm not clear exactly how all these routines get used. For classFileparser we should no concurrency issues. That seems reasonable. What degree of certainty does your “should” come with? :) Pretty high. We're parsing a stream of bytes and writing values into local structures that will eventually be passed across to a klass instance, which in turn will eventually be published via the SD as a loaded class. The actual parsing phase is purely single-threaded. David Cheers, Mikael David In the unaligned/memcpy case I think we can agree that there’s nothing preventing the compiler from doing individual loads/stores of the bytes making up the data. Especially in something like slowdebug that becomes more or less obvious - memcpy most likely isn’t intrinsified and is quite likely just copying a byte at a time. Given that the data is, in fact, unaligned, there is really no simple way to prevent word tearing, so I’m pretty sure that we never depend on it - if needed, we’re likely to already have some higher level synchronization in place guarding the accesses. And the fact that the other, non-x86 platforms already do individual byte loads/stores when the pointer is unaligned indicates is a further indication that that’s the case. However, the aligned case is where stuff gets more interesting. I don’t think the C/C++ spec guarantees that accessing a memory location using a pointer of type T will result in code which does a single load/store of size >= sizeof(T), but for all the compilers we actually use that’s likely to be the case. If it’s true that the compilers don’t splits the memory accesses, that means we won’t have word tearing when using the Bytes::get/put methods with aligned pointers. If I switch to always using memcpy, there’s a risk that it introduces tearing problems where earlier we had none. Two questions come to mind: * For the cases where the get/put methods get used today, is that a problem? * What happens if somebody in the future decides that putJavau4 seems like a great thing to use to write to a Java int field on the Java heap, and a Java thread is racing to read that same data? All that said though, I think this is worth exploring and it may well turn out that word tearing really isn’t a problem. Also, I believe there may be opportunities to further clean up this code and perhaps unify it a bit across the various platforms. And that said, I think the change as it stands is still an improvement, so I’m leaning towards pushing it and filing an enhancement and following up on it separately. Let me know if you strongly feel that this should be looked into and addressed now and I may reconsider :) Cheers, Mikael /Robbin ------------------------------------------------------------------------------

More information about the hotspot-dev mailing list