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

[Yasumasa Suenaga]

Thanks Vladimir!

Yasumasa


On 2017/07/13 23:32, Vladimir Ivanov wrote:
> 
> 
>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8184309/webrev.00/
>>
>> Looks good. I've submitted a JPRT build for testing.
> 
> FTR the job finished successfully. I'll wait for second review before the push.
> 
> 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?)
>>>>