[PATCH FOR REVIEW]: Fix warning in src/share/vm/adlc/archDesc.cpp

David Holmes - Sun Microsystems David.Holmes at Sun.COM
Mon Sep 7 17:37:33 PDT 2009 

Doh! The assert is wrong:

assert(count == size, "copyright info truncated");

should be:

assert(count == 1, "copyright info truncated");

as fwrite returns the number of items written, not the number of bytes.

Or, as David Schlosnagle pointed out, the assert would be correct if we 
reversed the "size" and "1" arguments passed to fwrite.

David Holmes

David Holmes - Sun Microsystems said the following on 09/08/09 10:32:
> David,
> 
> David Schlosnagle said the following on 09/08/09 09:53:
>> It seems like the warning and assert actually pointed out an existing
>> bug -- the second and third arguments to fwrite seem to be swapped.
>> The second arg should be the size (1 for char) and the third argument
>> should is the number of items to write.
> 
> I don't see how it makes a difference: size * 1 == 1 * size
> 
> All fwrite will do, from what I've seen, is multiply the two to obtain 
> the number of bytes to write.
> 
> It may be that Andrew encountered this bug: [BZ #5998]
> 
> http://sourceware.org/git/?p=glibc.git;a=commit;h=a7925a24fe104a2ab54fb8a6bdec1e5cf80a8db7 
> 
> 
> or maybe there really was an error writing the data out.
> 
> Cheers,
> David Holmes
> 
>>
>> The following should work:
>> //---------------------------addSUNcopyright------------------------------- 
>>
>>  // output SUN copyright info
>>  void ArchDesc::addSunCopyright(char* legal, int size, FILE *fp) {
>> -  fwrite(legal, size, 1, fp);
>> +  size_t count = fwrite(legal, 1, size, fp);
>> +  assert(count == size, "copyright info truncated");
>>    fprintf(fp,"\n");
>>    fprintf(fp,"// Machine Generated File.  Do Not Edit!\n");
>>    fprintf(fp,"\n");
>>  }
>>
>>> From <http://docs.sun.com/app/docs/doc/816-5168/fwrite-3c?a=view>:
>> Synopsis
>>     #include <stdio.h>
>>     size_t fwrite(const void *ptr, size_t size, size_t nitems, FILE 
>> *stream);
>>
>> Description
>>     The fwrite() function writes, from the array pointed to by ptr, up to
>>     nitems elements whose size is specified by size, to the stream 
>> pointed to
>>     by stream. For each object, size calls are made to the fputc(3C) 
>> function,
>>     taking the values (in order) from an array of unsigned char exactly
>>     overlaying the object. The file-position indicator for the stream (if
>>     defined) is advanced by the number of bytes successfully written. 
>> If an
>>     error occurs, the resulting value of the file-position indicator 
>> for the
>>     stream is unspecified.
>>
>>     The st_ctime and st_mtime fields of the file will be marked for 
>> update
>>     between the successful execution of fwrite() and the next successful
>>     completion of a call to fflush(3C) or fclose(3C) on the same 
>> stream or a
>>     call to exit(2) or abort(3C).
>>
>> Return Values
>>     The fwrite() function returns the number of elements successfully 
>> written,
>>     which might be less than nitems if a write error is encountered. 
>> If size or
>>     nitems is 0, fwrite() returns 0 and the state of the stream remains
>>     unchanged. Otherwise, if a write error occurs, the error indicator 
>> for the
>>     stream is set and errno is set to indicate the error.
>>
>> Thanks,
>> David Schlosnagle
>>
>> On Mon, Sep 7, 2009 at 6:40 PM, Andrew John Hughes
>> <gnu_andrew at member.fsf.org> wrote:
>>> 2009/9/6 Andrew John Hughes <gnu_andrew at member.fsf.org>:
>>>> 2009/9/5 David Holmes - Sun Microsystems <David.Holmes at sun.com>:
>>>>> Christian Thalinger said the following on 09/05/09 18:59:
>>>>>> Well, looking more closely it's actually a system header attribute 
>>>>>> that
>>>>>> brings up that warning:
>>>>>>
>>>>>>
>>>>>> /mnt/builder/icedtea7/openjdk-ecj/hotspot/src/share/vm/adlc/archDesc.cpp:1034: 
>>>>>>
>>>>>> error: ignoring return value of 'size_t fwrite(const void*, size_t,
>>>>>> size_t, FILE*)', declared with attribute warn_unused_result
>>>>>>
>>>>>> And it seems there is no GCC switch to turn that warning off (except
>>>>>> -O0).  Maybe we should check for the written bytes to be the same as
>>>>>> requested:
>>>>>>
>>>>>>  assert(count == size, "copyright info truncated");
>>>>> I recall this coming up in the past. gcc manual lists:
>>>>>
>>>>> -Wno-unused-result
>>>>>
>>>>> but that might be version specific.
>>>>>
>>>>> David
>>>>>
>>>> It's a very recent addition:
>>>>
>>>> http://gcc.gnu.org/ml/gcc-patches/2009-07/msg00474.html
>>>>
>>>> which means there is a period (which includes GCC 4.4) where the
>>>> warning is produced but can't be disabled :(
>>>>
>>>> BTW, my current approach was influenced by
>>>> http://hg.openjdk.java.net/jdk7/jdk7/hotspot/rev/2328d1d3f8cf where
>>>> the same fix is applied to remove this warning.
>>>> I have to say I personally prefer twisti's assert fix and can apply
>>>> this if others are in agreement.
>>>> -- 
>>>> Andrew :-)
>>>>
>>>> Free Java Software Engineer
>>>> Red Hat, Inc. (http://www.redhat.com)
>>>>
>>>> Support Free Java!
>>>> Contribute to GNU Classpath and the OpenJDK
>>>> http://www.gnu.org/software/classpath
>>>> http://openjdk.java.net
>>>>
>>>> PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
>>>> Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8
>>>>
>>> This is interesting; that assertion fails:
>>>
>>> g++ -m64 -Xlinker -O1  -m64 -export-dynamic -o
>>> ../generated/adfiles/adlc ../generated/adfiles/adlparse.o
>>> ../generated/adfiles/archDesc.o ../generated/adfiles/arena.o
>>> ../generated/adfiles/dfa.o ../generated/adfiles/dict2.o
>>> ../generated/adfiles/filebuff.o ../generated/adfiles/forms.o
>>> ../generated/adfiles/formsopt.o ../generated/adfiles/formssel.o
>>> ../generated/adfiles/main.o ../generated/adfiles/adlc-opcodes.o
>>> ../generated/adfiles/output_c.o ../generated/adfiles/output_h.o
>>> ../generated/adfiles/adlc -DLINUX -D_GNU_SOURCE -DAMD64 -q -T -D_LP64
>>> ../generated/adfiles/linux_x86_64.ad  \
>>>  -c../generated/adfiles/mktmp11816/ad_x86_64.cpp
>>> -h../generated/adfiles/mktmp11816/ad_x86_64.hpp
>>> -a../generated/adfiles/mktmp11816/dfa_x86_64.cpp
>>> -v../generated/adfiles/mktmp11816/adGlobals_x86_64.hpp \
>>>            || { rm -rf ../generated/adfiles/mktmp11816; exit 1; }
>>> assert fails 
>>> /mnt/builder/icedtea7/openjdk-ecj/hotspot/src/share/vm/adlc/archDesc.cpp
>>> 1035: copyright info truncated
>>> /bin/sh: line 2: 12016 Aborted
>>> ../generated/adfiles/adlc -DLINUX -D_GNU_SOURCE -DAMD64 -q -T -D_LP64
>>> ../generated/adfiles/linux_x86_64.ad
>>> -c../generated/adfiles/mktmp11816/ad_x86_64.cpp
>>> -h../generated/adfiles/mktmp11816/ad_x86_64.hpp
>>> -a../generated/adfiles/mktmp11816/dfa_x86_64.cpp
>>> -v../generated/adfiles/mktmp11816/adGlobals_x86_64.hpp
>>> make[7]: *** [refresh_adfiles] Error 1
>>>
>>> -- 
>>> Andrew :-)
>>>
>>> Free Java Software Engineer
>>> Red Hat, Inc. (http://www.redhat.com)
>>>
>>> Support Free Java!
>>> Contribute to GNU Classpath and the OpenJDK
>>> http://www.gnu.org/software/classpath
>>> http://openjdk.java.net
>>>
>>> PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
>>> Fingerprint: F8EF F1EA 401E 2E60 15FA  7927 142C 2591 94EF D9D8

More information about the hotspot-dev mailing list