RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.

[Lindenmaier, Goetz]

Hi Kim,

I started to fix the issues you noted, but I'm not sure about this one: 
> - Result of final jio_snprintf is not checked.
I think it's on purpose that the result is not checked.  In case of an error
of the VM, one wants to see as much information as possible. The result 
of get_core_path is only used for printing, and printing a truncated path 
still might help to find the core.
That's also the reason to return strlen and not the result of jio_snprintf.

Note that jio_snprintf always terminates strings and returns -1 on error.

Best regards,
  Goetz.



-----Original Message-----
From: Kim Barrett [mailto:kim.barrett at oracle.com] 
Sent: Thursday, February 26, 2015 11:25 PM
To: Lindenmaier, Goetz
Cc: hotspot-dev Source Developers
Subject: Re: RFR(M): 8073315: Enable gcc -Wtype-limits and fix upcoming issues.

On Feb 19, 2015, at 5:34 AM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com> wrote:
>> src/os/linux/vm/os_linux.cpp  
>> 6028   int written;
>> ...
>> 6050   if ((written >= 0) && ((size_t)written < bufferSize) &&
>> 6051       (pid_pos == NULL) && (core_pattern[0] != '|')) {
>> 
>> The real problem here is the value of "written", which is the result
>> of a call to jio_snprintf, is not being checked for failure. Clearly
>> "written" should be of signed type because of its value, but the other
>> changes here just continue to mask the lack of error checking.
> 
> os_linux.cpp:
>  - I adapted the type to ptrdiff_t.

Thanks, that looks good.

>  - I implemented error handling as where current directory can not be read.

5999 // Returns 0 and sets buffer to the empty string in case of failure.
6000 int os::get_core_path(char* buffer, size_t bufferSize) {
... 
6008   if (bufferSize == 0) { return 0; }
...
6052   if (written < 0) {
6053     assert(false, "Could not write core_pattern to buffer");
6054     buffer[0] = '\0';
6055     return 0;
6056   }

Based on the API, I think the intent (or at least the stylistically
consistent behavior) is to have get_core_path return -1 to indicate
failure, with the buffer contents unspecified in that case.  The one
caller of this function would operate correctly with that behavior.

So rather than documenting this variant's error behavior, I think the
declaration in runtime/os.hpp should be augmented with error behavior,
and I think it ought to return -1 for errors.

The assert(false, ...) is inappropriate.  asserts should not be used
for reporting external issues of this sort.  Similarly for the earlier
pre-existing assert on the result of get_current_directory.  Just
return the error flag.

There are more pre-existing error checking bugs here that I'm going to
suggest you not try to fix as part of this change set, but instead
report as a new bug.  Specific things I noticed

- Result of final jio_snprintf is not checked.
- Result of ::close() is not checked.  Consider EINTR.
- Result of ::read() is not checked.

I suspect the last two are endemic in this code base.

And every platform-specific implementation of get_core_path has
similar error checking issues in varying degrees.  (I looked at all of
them and quickly spotted bugs in each one.)

------------------------------------------------------------------------------ 
src/share/vm/opto/graphKit.cpp
3048     if (spec_obj_type != NULL || (data != NULL)) {

Please use consistent parenthesization for the two sides of the ||.

>> ---------------------------------------------------------------------------
>> src/share/vm/gc_implementation/g1/concurrentMark.cpp
>> 2173 #ifndef PRODUCT
>> 2174       if (G1StressConcRegionFreeing) {
>> 2175         for (uintx i = 0; i < G1StressConcRegionFreeingDelayMillis; ++i) {
>> 2176           os::sleep(Thread::current(), (jlong) 1, false);
>> 2177         }
>> 2178       }
>> 2179 #endif
>> 
>> The #ifndef here is the kind of workaround for the warning that I
>> really dislike.
> 
> You're right, that #define is not nice. And yes, the code you propose
> is better. But here I don't feel like changing it, as I don't know
> what was intended by the code. Is it used at all? Does Oracle have a
> test that depends on it? Anyways, if the default is 0, it's off per
> default if somebody enables G1StressConcRegionFreeing, which I also
> consider a strange behaviour. The code was added with 6977804: G1:
> remove the zero-filling thread:
> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/0fa27f37d4d4 I can't
> find the review thread in the gc mail archive. There is only the
> submit:
> http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2011-January/002243.html

I still don't like it, but agree the #ifndef is the appropriate change
for this changeset.  Maybe a followup RFE suggesting cleanup of the
loop?