Hi Yasumasa,
A couple of comments on (B). First there is a typo in
"stringop-truncatino". Second, can the macros be used just before and
after the strncpy reference rather than around the whole function? It
would clarify both that the strncpy use has these macros in place and
that the macros are there for the strncpy. As it stands now, if you see
the macro references it won't be readily obvious why they are there.
As for which I prefer, I'm leaning towards (A) for it's simplicity. Yes,
I understands Kim's reason for opposing it, but (B) and (C) are no
better in that regard. In all cases we are doing something that should
be completely unnecessary to work around a gcc bug. I could see
preferring (C) if we are concerned about this issue popping up in other
places.
BTW, I had mentioned seeing another instance of this recently in a
review I did:
http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-September/029361.html
I was thinking of pushing back on having to fix a perfectly valid
strncpy, and like Kim was not that happy about having to switch it to a
memcpy. I considered suggesting disabling the warning but decided that
for this one issue bowing to memcpy was probably best, so I never
brought it up. So following along on those same thoughts, for your fix
memcpy might be acceptable (it's just "one" case if we ignore already
fixed cases), but disabling the warning is probably best if we think
this is going to be an issue that continues to pop up.
thanks,
Chris
On 10/11/19 6:38 AM, Yasumasa Suenaga wrote:
Hi,
Christos commented to B-1. Thanks!
clang defines __GNU_C__ , but stringop-truncation is not supported.
So I updated Plan B-1. It works fine on my Fedora30 box.
A. Use memcpy()
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.02/
B-1. Use #pragma
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.B1-pragma.02/
C. Set -Wno-stringop-truncation in globally
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.C/
Of course I will push the change to submit repo before sending review
request.
Thanks,
Yasumasa
On 2019/10/11 15:55, Yasumasa Suenaga wrote:
Hi,
Thanks for a lot advises!
We have following solutions for this issue.
I'd like to send RFR again with much consented patch in early next week.
A. Use memcpy()
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.02/
B-1. Use #pragma
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.B1-pragma/
C. Set -Wno-stringop-truncation in globally
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.C/
If we fix with C, I will send RFR to hotspot-dev and build-dev.
Plan C also fixes other stringop-truncation problems such as
JDK-8220074.
Thus it affects all of JDK code, but it would be useful if
stringop-truncation
should be disabled in JDK build process.
Comments are welcome.
Yasumasa
On 2019/10/11 10:34, Yasumasa Suenaga wrote:
Hi,
I want to get conclusion of this discussion.
I understand the fix of macroAssembler_x86.hpp is ok, but we have
not yet had conclusion
how we should fix diagnosticArgument.cpp .
I think we can fix diagnosticArgument.cpp as following:
A. Use memcpy()
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.02/
B. Add -Wno-stringop-truncation to
make/hotspot/lib/JvmOverrideFiles.gmk
This option will be added diagnosticArgument.cpp only.
C. Set -Wno-stringop-truncation in globally
make/hotspot/lib/CompileJvm.gmk
I prefer to fix like A because it affects minimally.
Some issues might be found out by stringop-truncation in future.
Thanks,
Yasumasa
On 2019/10/11 5:54, Kim Barrett wrote:
On Oct 10, 2019, at 3:03 AM, David Holmes
<david.hol...@oracle.com> wrote:
On 10/10/2019 4:50 pm, Chris Plummer wrote:
From JBS:
/home/ysuenaga/OpenJDK/jdk/src/hotspot/share/services/diagnosticArgument.cpp:154:14:
warning: 'char* strncpy(char*, const char*, size_t)' output
truncated before terminating nul copying as many bytes from a
string as its length [-Wstringop-truncation]
154 | strncpy(buf, str, len);
| ~~~~~~~^~~~~~~~~~~~~~~
I assume this means that in all cases the "len" value is seen to
be derived from strlen, and therefore strncpy is always copying
one byte short of \0, and this is most likely not what the user
wants. I seem to
Yes but we then explicitly set the NULL at buf[len] which is the
expected/required pattern for this.
recall another recent similar fix that was done by switching to
using memcpy instead.
Here's a discussion of interest, also suggesting memcpy:
https://stackoverflow.com/questions/50198319/gcc-8-wstringop-truncation-what-is-the-good-practice
Seems to me that strncpy and memcpy are semantically equivalent
here so all this does is avoid gcc's over zealous warnings. I'm
inclined to use the:
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-truncation"
solution.
YMMV.
We've run into and discussed problems with -Wstringop-truncation
before. (See discussions of JDK-8214777 and JDK-8223186.) This is a
relatively recent warning option (introduced in gcc8, and included in
-Wall), and seems to have a considerable bug tail:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88781
A metabug for -Wstringop-truncation, currently with 16 open and 10
resolved associated bugs.
I'm not a fan of replacing correct and idiomatic uses of strncpy with
strcpy or memcpy. I've suggested in the past that we should turn off
this warning while it is so buggy.
