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

Andrew John Hughes gnu_andrew at member.fsf.org
Mon Sep 7 18:35:08 PDT 2009 

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