Thanks! That's seems to be a nicer way to fix it, but remember that both gcc and clang define __GNUC__ so I think this patch should be more restrictive:
http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.B1-pragma/src/hotspot/share/utilities/compilerWarnings_gcc.hpp.udiff.html <http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.B1-pragma/src/hotspot/share/utilities/compilerWarnings_gcc.hpp.udiff.html> christos > On Oct 11, 2019, at 8:57 AM, Yasumasa Suenaga <suen...@oss.nttdata.com> wrote: > > Hi, > > I proposed 3 solutions in [1]. > > Plan B-1 is #pragma, but HotSpot provides valid macro to compiler. > I added stringop-truncation to it [2], and it would work each compilers. > > > Yasumasa > > > [1] > https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-October/029580.html > [2] http://cr.openjdk.java.net/~ysuenaga/JDK-8232084/webrev.B1-pragma/ > > > On 2019/10/11 21:51, David Holmes wrote: >> On 11/10/2019 10:47 pm, Christos Zoulas wrote: >>> At NetBSD we decided to change the Makefiles because we compile with both >>> clang and gcc >>> and clang does not have those warnings and it complains about the pragmas. >> <sigh> Compilers are supposed to ignore pragmas they don't understand. >> Complaining about an unknown pragma defeats the whole purpose of having >> pragmas. >> I suppose to prevent problems with clang we must disable the warning in the >> makefile. >> David >>> christos >>> >>>> On Oct 11, 2019, at 2:54 AM, David Holmes <david.hol...@oracle.com> wrote: >>>> >>>> Yes I prefer: >>>> >>>> D. Use #pragma ... at the callsite to disable this particular diagnostic. >>>> >>>> over changing the makefiles. >>>> >>>> David >>>> >>>> On 11/10/2019 3:37 pm, Ioi Lam wrote: >>>>> For diagnosticArgument.cpp, I would prefer adding inline #pragma at only >>>>> the sites affected (as suggested by David Holmes), with a comment about >>>>> why this is necessary. >>>>> But if others decide to go with other solutions, that's fine with me, >>>>> too, but please remember to add a comment :-) >>>>> Thanks >>>>> - Ioi >>>>> On 10/10/19 6:50 PM, Chris Plummer wrote: >>>>>> These solutions all have their merits and their warts, but we have to >>>>>> pick something. I'm ok with with A or C. For B I'd rather you instead >>>>>> used the #pragma at the warning site. If you go with (C), the review >>>>>> should go out to all of hotspot-dev and build-dev. >>>>>> >>>>>> Chris >>>>>> >>>>>> On 10/10/19 6:34 PM, 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. >>>>>>>> >>>>>> >>>>>> >>>
