Hi Chris,
thanks again for your reply.
> shmemBase.c: seems correct, but could use explanation/example of when
> existing code was failing.
This is a coverity finding. The correct size specification as per Microsoft’s
doc [1] for jlong (__int64) would be I64.
Yes. I was just wondering if using the incorrect format ever produced a test
failure. Also, isn't %ld incorrect for 32-bit platforms? But then I doubt this
code has ever been compiled for a 32-bit unix target. Also, it looks like
PRId64 should work for all platforms. This is what we use in hotspot to print a
64-bit value:
#define INT64_FORMAT "%" PRId64
So maybe this fix can be simplified.
You are probably right, I’ll address this in my next webrev.
VirtualMachineImpl.c: When/why is pos ever null?
This is also a coverity case. For some reason, coverity thinks that pos could
be NULL here. But I’m with you – I also don’t see a path how that could ever
happen. Nevertheless, this change would please converity and I don’t see it
being performance critical so I would like to leave it. It’s also not wrong.
I'm actually more concerned about making the code harder to read and misleading
the reader into thinking it could actually be NULL. I'm not familiar with
coverity, but I thought tools like this either provided a specific call path
for these types of errors (in which case the problem might actually be up the
call chain), or allowed you to configure the tool not to complain about
specific cases that you know can't happen.
OK, what I’m gonna do now: I’ve reverted the stuff in our codebase to get a new
coverity run. If coverity still complains I’ll check again to first of all get
an explanation by coverity what should be wrong and then find a more sensible
fix. Or maybe we can tell coverity to shut up…
error_messages.c: Is there any reason not to do this fix for all platforms? Do
you have a test case for it?
This is a Windows only issue. “vsnprintf“ would write the terminating 0. But in
jdk.jdwp.agent/windows/native/libjdwp/util_md.h, vsnprintf is redefined to
_vsnprintf. And this, by the Mircrosoft documentation [2], doesn’t necessarily
write the terminating character.
Any idea why we do this redefine of vsnprintf? In any case, it might be best to
just unconditionally terminate rather than have WIN32 explicit code.
After reading the specs again, I think that windows uses _vsnprintf since this
version matches the standard Unix/Linux behavior better which means that the
buffer is written up to count and a terminating 0 might not be written. So it’s
best to terminate the buffer in any case which will be reflected by my new
webrev.
eventHelper.c: Do you have a testcase? Also, is the "bagAdd(eventBag)" message
going to be of use to the user?
For this one I don’t have the history. Maybe it was also the coverity tool. But
there is no testcase. In my update I have updated 2 other places with the null
check where bagAdd is called. I guess in case of such a null error occurring,
it could help a little. But probably this would be something to be investigated
by the developer rather than the debugging user.
I think you should hold off on this one until you have a better handle on why
this was needed, and possible also include a test case.
This one is coverity, too. I’m now doing the same approach like I do with
VirtualMachineImpl.c. I’m removing the stuff and see if coverity will complain
again and then rethink the patch.
inStream.c: inStream_init() now uses a magic number of 11. What does it
represent? inStream_endOfInput() looks like it used to be completely broken.
How was this not noticed? Is this related to the change inStream_init(), which
possible was masking this error? Is there a test case for it?
I found this fix in our code base. instream_endOfInput() is obviously never
used. At least I don’t find any references in the source code scan. Maybe I
shall completely remove it?
As for the “magic number of 11”: This is the offset to the payload of a
jdwpCmdPacket, see jdk.jdwp.agent/share/native/include/ jdwpTransport.h.
(sizeof(jint) /* len*/ + sizeof(jint) /* id */ + sizeof(jbyte) /* flags*/ +
sizeof(jbyte) /* cmdSet */ + sizeof(jbyte) /*cmd */
Maybe I should write it down explicitly?
Can you use offsetof?
Yes, will do in next webrev.
I’m still hoping to get this change done under the hood of JDK-8192978. I will
update the bug with some more details and also add an appropriate noreg label.
I think the coverity fixes can be one bug. The format changes one bug (I assume
coverity did not find them). inStream.c should have its own bug. eventHelper.c
should have its own bug and more investigation as to why it is needed. I get
nervous when the reason for a change has been forgotten.
Ok, I will eventually split as suggested.
So, I’ll check the coverity results tomorrow and then come up with new webrevs.
Best regards,
Christoph
