On Wed, 2012-05-23 at 09:42 -0400, Keith McGuigan wrote: > >> The first patch is just a consistency cleanup patch. The JNI Set and > >> SetStatic Field methods used HS_DTRACE_PROBE_CDECL_N and HS_DTRACE_PROBE_N > >> directly instead of just using DTRACE_PROBE[N] like all other JNI methods. > >> This doesn't matter for the Solaris macros, but on GNU/Linux the macros > >> don't use direct function declarations (which is introduced in the second > >> patch). > > Ok, might be that it's fine. I don't remember if there was a reason > that SetStatic was different from the other JNI calls, but it certainly > could just have been an omission in refactoring or something. We just > need to make sure that it still works (solaris-dtrace-wise). It's > certainly better this way!
Yeah, that is why I originally split out the patches, so that anything that might potentially impact other architectures could be applied and tested separately. Especially since I don't have access to solaris. > >> dtrace.hpp should be refactored (if possible) into the os-specific > >> subdirectories, instead of using #ifdef<OS> macros in the header. > >> I.e., you might have to make a src/solaris/vm/dtrace_solaris.hpp and > >> src/linux/vm/dtrace_linux.hpp file and include it from here. > > > > I rather not, the code should be identical except for two small details > > between solaris and gnu/linux as the patch shows (and one is just a hack > > for a solaris tailcall bug). The apple/macos part looks very different > > and could be split out, but that was already there. > > I refer to the redefinition blocks of HS_DTRACE_PROBEx, which I don't > consider a small detail (though the solaris tailcall thing certainly > is). > > It's (of course) just a style thing, but traditionally in hotspot > we've wanted the os or arch specific code in os or arch specific > directories, instead of littering the code with #ifdefs. I know the OSX > stuff started violated this some, but I hope we're going to resolve that > rather than continue down that path. I wouldn't have mind if the OSX stuff was split out this way, but now I don't have any example to follow here. What/How do you suggest it is done? Thanks, Mark
