On Fri, Nov 29, 2019 at 8:59 AM Baesken, Matthias <matthias.baes...@sap.com> wrote:
> Hi Thomas, probably jplis_assert(x) should better be named > jplis_warn(x) . > > > Yes :-) Additionally the TRANSFORM macro could be enhanced by an abort() call (or abortJVM <http://ld8443:8080/source/s?defs=abortJVM&project=openjdk-jdk> > > ?) or something similar . > > > I think that makes sense, or one could even make jplis_assert abort() the program. But I leave this up to you, could also be done in a different issue. The patch you posted looks fine to me as it is. Cheer,s Thomas Best regards, Matthias > > > > > > > > > > > > Just read Matthias reply: > > > > We call jplis_assert() if allocation fails. Looking at > > > > src/java.instrument/share/native/libinstrument/JPLISAssert.h > > > > I see that these assertions seem to be turned on all the time: > > > > 45 #define JPLISASSERT_ENABLEASSERTIONS (1) > > > > and lands us in JPLISAssertCondition() (possible improvement here is to > evaluate the condition before the call): > > > > 58 #define jplis_assert(x) > JPLISAssertCondition((jboolean)(x), #x, THIS_FILE, __LINE__) > > > > However, JPLISAssertCondition() is not an assert - name is misleading - > but just a printf(): > > > > 39 void > 40 JPLISAssertCondition( jboolean condition, > 41 const char * assertionText, > 42 const char * file, > 43 int line) { > 44 if ( !condition ) { > 45 fprintf(stderr, "*** java.lang.instrument ASSERTION FAILED > ***: \"%s\" at %s line: %d\n", > 46 assertionText, > 47 file, > 48 line); > 49 } > 50 } > > > > Maybe I miss something but I do not see an abort. > > > > ---- > > > > I think we should add an exit(2) or abort(2) to the assertion. > > > > But I also think this is a different issue from what Matthias tries to > fix, so I am fine with Matthias change. > > > > Cheers, Thomas > > > > > > > > > > > > On Fri, Nov 29, 2019 at 8:20 AM Alan Bateman <alan.bate...@oracle.com> > wrote: > > On 29/11/2019 06:29, Thomas Stüfe wrote: > > Hi Matthias, > > > > I am not certain the callers are prepared to handle NULL. > > > > This is used in a chain of TRANSFORM macro calls which AFAICS do not > > handle NULL; e.g. , at 872, we pass the returned pointer to > > convertUft8ToPlatformString which passes it on (on Windows) to > > MultiByteToWideChar, which does not handle NULL input. > > > > So I wonder whether a clear error message with an exit would be better > > in this case. Otherwise we may get a crash just some instructions later. > > > Right, this needs a lot more analysis to see if it's even possible to > continue. The main usage is VM startup where the -javaagent option > specifies agents that have the Boot-Class-Path attribute. In that case > it would not be unreasonable to abort the process, it's unlikely to get > much startup in the startup if memory is exhausted. The other possible > context is where a tool agent is loaded into a running VM, in which case > have the attach thread return with a pending exception might be okay > (although the VM is likely to shutdown anyway as the memory exhaustion > will be detected/handled elsewhere). > > -Alan > >
