[PATCH FOR REVIEW]: Fix warning in src/share/vm/adlc/archDesc.cpp
David Holmes - Sun Microsystems
David.Holmes at Sun.COM
Mon Sep 7 18:13:11 PDT 2009
Andrew,
Andrew John Hughes said the following on 09/08/09 10:51:
> 2009/9/8 David Holmes - Sun Microsystems <David.Holmes at sun.com>:
>> 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.
>>
>
> Yeah, the assert or the fwrite call are wrong, depending on which way
> you look at it. Generally, you'd want to check that all the
> characters were written out, rather than checking that it returns 1,
> so I think the assert is right and the fwrite should be changed.
> That's what the webrev:
>
> http://cr.openjdk.java.net/~andrew/werror/webrev.02/
>
> does.
Doesn't this:
void ArchDesc::addSunCopyright(char* legal, int size, FILE *fp) {
! size_t count = fwrite(legal, 1, size, fp);
! assert(count == size, "copyright info truncated");
generate a signed vs. unsigned comparison warning ?
> But it's nice that fixing the warning by adding a check
> does catch this, and if fwrite ever fails, we now won't ignore it.
>
> Ok to push?
I guess that as this is only a build-time issue then using the assert is
ok. But the cynic in me won't be surprised if we find we get unexpected
failures. That said, I'm not even sure if assertions are enabled during
the build process ...
> If so, I need a bug ID for it :)
6879689 (hope I did it right! not quite sure of the protocol for these :) )
David Holmes
>> 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