[10] RFR: 8184309: Buld warnings from GCC 7.1 on Fedora 26

[Yasumasa Suenaga]

Thanks Kim, Vladimir,

> Could this one instead be fixed by changing the declaration of TimestampFormat from
>
> static const char*  TimestampFormat;
>
> to
>
> static const char*  const TimestampFormat;

It works fine.
So I use it.

I filed this issue as JDK-8184309, and uploaded webrev.
It does not contain the warning of methodMatcher.cpp .
Could you review it?

http://cr.openjdk.java.net/~ysuenaga/JDK-8184309/webrev.00/


I cannot access JPRT.
So I need a sponsor.


Yasumasa


2017-07-13 3:30 GMT+09:00 Kim Barrett <kim.barrett at oracle.com>:
>> On Jul 11, 2017, at 9:51 PM, Yasumasa Suenaga <yasuenag at gmail.com> wrote:
>>
>> Hi all,
>>
>> I tried to build jdk10-hs on Fedora 26 with gcc-7.1.1-3.fc26.x86_64,
>> then I encountered some build warnings.
>> I want to fix them. Can I file it to JBS and send review request for 10?
>
> Yes to filing issues and submitting patches or RFRs.
>
> A couple of comments on the proposed patches.
>
>> 2-warning:
>> /home/ysuenaga/OpenJDK/jdk10-hs/hotspot/src/share/vm/compiler/methodMatcher.cpp:
>> In static member function 'static bool
>> MethodMatcher::canonicalize(char*, const char*&)':
>> /home/ysuenaga/OpenJDK/jdk10-hs/hotspot/src/share/vm/compiler/methodMatcher.cpp:99:22:
>> warning: comparison between pointer and zero character constant
>> [-Wpointer-compare]
>>     if (colon + 2 != '\0') {
>>                      ^~~~
>> /home/ysuenaga/OpenJDK/jdk10-hs/hotspot/src/share/vm/compiler/methodMatcher.cpp:99:22:
>> note: did you mean to dereference the pointer?
>>
>> 2-patch:
>> diff -r 9c54cd2cdf09 src/share/vm/compiler/methodMatcher.cpp
>> --- a/src/share/vm/compiler/methodMatcher.cpp   Mon Jul 10 23:28:25 2017 +0200
>> +++ b/src/share/vm/compiler/methodMatcher.cpp   Wed Jul 12 10:32:20 2017 +0900
>> @@ -96,7 +96,7 @@
>>   bool have_colon = (colon != NULL);
>>   if (have_colon) {
>>     // Don't allow multiple '::'
>> -    if (colon + 2 != '\0') {
>> +    if (colon[2] != '\0') {
>>       if (strstr(colon+2, "::")) {
>>         error_msg = "Method pattern only allows one '::' allowed";
>>         return false;
>
> Already reported in JDK-8181503.  There’s an RFR out already, though it’s been stalled
> by discussion of one of the other bullet items in that CR plus vacations.
>
>> 3-warning:
>> /home/ysuenaga/OpenJDK/jdk10-hs/hotspot/src/share/vm/logging/logFileOutput.cpp:
>> In static member function 'static void
>> LogFileOutput::set_file_name_parameters(jlong)':
>> /home/ysuenaga/OpenJDK/jdk10-hs/hotspot/src/share/vm/logging/logFileOutput.cpp:61:99:
>> warning: format not a string literal, format string not checked
>> [-Wformat-nonliteral]
>>   res = (int)strftime(_vm_start_time_str, sizeof(_vm_start_time_str),
>> TimestampFormat, &local_time);
>>
>>                            ^
>>
>> 3-patch:
>> diff -r 9c54cd2cdf09 src/share/vm/logging/logFileOutput.cpp
>> --- a/src/share/vm/logging/logFileOutput.cpp    Mon Jul 10 23:28:25 2017 +0200
>> +++ b/src/share/vm/logging/logFileOutput.cpp    Wed Jul 12 10:32:20 2017 +0900
>> @@ -51,6 +51,8 @@
>>   _file_name = make_file_name(name + strlen(Prefix), _pid_str,
>> _vm_start_time_str);
>> }
>>
>> +PRAGMA_DIAG_PUSH
>> +PRAGMA_FORMAT_NONLITERAL_IGNORED
>> void LogFileOutput::set_file_name_parameters(jlong vm_start_time) {
>>   int res = jio_snprintf(_pid_str, sizeof(_pid_str), "%d",
>> os::current_process_id());
>>   assert(res > 0, "PID buffer too small");
>> @@ -61,6 +63,7 @@
>>   res = (int)strftime(_vm_start_time_str, sizeof(_vm_start_time_str),
>> TimestampFormat, &local_time);
>>   assert(res > 0, "VM start time buffer too small.");
>> }
>> +PRAGMA_DIAG_POP
>>
>> LogFileOutput::~LogFileOutput() {
>>   if (_stream != NULL) {
>
> Could this one instead be fixed by changing the declaration of TimestampFormat from
>
> static const char*  TimestampFormat;
>
> to
>
> static const char*  const TimestampFormat;
>
> with a corresponding update of the definition?  Some web searching suggests that
> should work, and if it does, I’d much prefer it to the warning suppression.
>
> If that doesn’t work, then the warning suppression should be commented to indicate
> exactly what the problem is, e.g. that TimestampFormat is triggering -Wformat-nonliteral.
> Otherwise, it’s not immediately obvious where the problem lies, and the warning
> control scope being the entire function doesn’t help.  (I don’t remember, can the
> warning scope be narrowed to just the offending statement?)
>