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

[Vladimir Ivanov]

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

Looks good. I've submitted a JPRT build for testing.

Best regards,
Vladimir Ivanov

> 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?)
>>