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

[Kim Barrett]

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?