[PATCH FOR REVIEW]: Fix warning in src/share/vm/adlc/archDesc.cpp
David Schlosnagle
schlosna at gmail.com
Mon Sep 7 19:14:31 PDT 2009
Is it worthwhile to change the method signature to better identify its
arguments?
void ArchDesc::addSunCopyright(const char* legal, size_t size, FILE *fp) {
size_t count = fwrite(legal, sizeof(char), size, fp);
assert(count == size);
fprintf(fp,"\n");
fprintf(fp,"// Machine Generated File. Do Not Edit!\n");
fprintf(fp,"\n");
}
Its a slippery slope to start checking the return values on those I/O
functions, and it doesn't look like anything else in that file is
checking them. I'm definitely not the right person to say whether the
assert is the proper error handling for this situation, but I'd assume
the user should know that the files weren't able to be written (most
likely due to some larger I/O problem) rather than fail silently.
Thanks,
Dave
On Mon, Sep 7, 2009 at 9:35 PM, Andrew John
Hughes<gnu_andrew at member.fsf.org> wrote:
> 2009/9/8 David Holmes - Sun Microsystems <David.Holmes at sun.com>:
>> 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 ?
>>
>
> Interestingly, it doesn't, even though int is signed and size_t is
> unsigned. That's a good point.
> I can add a cast of size to size_t to be on the safe side. That
> happens anyway in passing it to fwrite, as its arguments are const
> void*, size_t, size_t, FILE*.
>
>>> 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 ...
>>
>
> They are at some point, that's what caused the previous failure :)
> That also implies the code inside the assert is getting compiled.
>
>>> If so, I need a bug ID for it :)
>>
>> 6879689 (hope I did it right! not quite sure of the protocol for these :) )
>>
>
> Thanks! If I create one as an external user, I'm told to wait three
> days. Last time, even that wasn't good enough and I had to get
> Dalibor to push some buttons internally.
>
>> 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
>>>
>>>
>>>
>>
>
>
>
> --
> 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