bugFreeciv - Bugs: bug #20413, cityrepdata.c: split_string():...

 
 
Show feedback again

bug #20413: cityrepdata.c: split_string(): 'd.string_value' may be used uninitialized in this function

Submitted by:  Roland Haeder <quix0r>
Submitted on:  Wed Jan 9 04:43:45 2013  
 
Category: clientSeverity: 3 - Normal
Priority: 5 - NormalStatus: Fixed
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Release: trunkOperating System: GNU/Linux
Planned Release: 2.3.4, 2.4.0, 2.5.0Contains string changes: None

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

Tue Jan 22 16:42:04 2013, SVN revision 22184:

Made datum value union of possible types instead of having them exist
parallel, but used one at a time. This also fixes compiler warnings
caused by initializing only the used value, not others.

Compiler warning reported by Roland Haeder

See gna bug #20413

(Browse SVN revision 22184)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue Jan 22 16:42:00 2013, SVN revision 22183:

Made datum value union of possible types instead of having them exist
parallel, but used one at a time. This also fixes compiler warnings
caused by initializing only the used value, not others.

Compiler warning reported by Roland Haeder

See gna bug #20413

(Browse SVN revision 22183)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue Jan 22 16:41:22 2013, SVN revision 22182:

Made datum value union of possible types instead of having them exist
parallel, but used one at a time. This also fixes compiler warnings
caused by initializing only the used value, not others.

Compiler warning reported by Roland Haeder

See gna bug #20413

(Browse SVN revision 22182)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sun Jan 20 20:56:46 2013, comment #6:

What compiler was complaining was that datum with numeric value, and string value uninitialized, was copied to vector. What it didn't see was that users of the vector always respect "is_numeric" boolean and never access string value of such a datum. (If it had seen real problem, it would have pointed the actual use of the uninitialized value, not where it got copied).

Still, it doesn't make sense to have separate numerical value and string value for a datum when there's ever only one of them used. Attached patch makes them an union (minimal optimization to both memory and CPU usage (as structure is copied around)). As union always gets initialized with either numerical or string value, this fixes also the original uninitialized value warning.

(file #16983)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat Jan 19 09:16:24 2013, comment #5:

> At this point I wouldn't rule compiler bug out. It gives the
> warnings only when it has refactored code heavily (optimization
> level 3) itself first.


But this is not proof of compiler bug either. It can be simply that code rearrangement makes it easier for warning heuristics to spot the problem. I just figured out one such a valid warning that appeared only with -O3 (probably related to more aggressive inlining - called function did not always set what caller depended on)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Sat Jan 19 07:59:14 2013, comment #4:

Combination of -O3 and --enable-debug seems to be poison for freeciv compilation. I didn't get nearly as far as you did before first "maybe-uninitialized" warning.

At this point I wouldn't rule compiler bug out. It gives the warnings only when it has refactored code heavily (optimization level 3) itself first.

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Tue Jan 15 00:44:56 2013, comment #3:

See attached files, they should help you to debug. I used latest trunk code at revision 22125.

Roland Haeder <quix0r>
Mon Jan 14 23:47:56 2013, comment #2:

> gcc (Debian 4.7.2-4) 4.7.2


Interestingly, this is just what I'm using and still not getting this warning/error. What's your arch? I'm on x86_64. Optimization level (-O?) Anyh other CFLAGS? configure options?

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon Jan 14 08:14:38 2013, comment #1:

bug #20219 has the same problem. (I'm inclined to make it duplicate of this one even though it's the older one - this one has useful summary)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed Jan 9 04:43:45 2013, original submission:

Compiling latest trunk results in:
------------------------------------------------------------
distcc[26446] ERROR: compile ../../../freeciv/client/cityrepdata.c on 192.168.2.3 failed
distcc[26446] (dcc_build_somewhere) Warning: remote compilation of '../../../freeciv/client/cityrepdata.c' failed, retrying locally
distcc[26446] Warning: failed to distribute ../../../freeciv/client/cityrepdata.c to 192.168.2.3, running locally instead
In file included from ../../../freeciv/client/cityrepdata.c:843:0:
../../../freeciv/client/cityrepdata.c: In function 'split_string':
../../../freeciv/utility/specvec.h:129:29: error: 'd.string_value' may be used uninitialized in this function [-Werror=maybe-uninitialized]
../../../freeciv/client/cityrepdata.c:956:20: note: 'd.string_value' was declared here
cc1: all warnings being treated as errors
distcc[26446] ERROR: compile ../../../freeciv/client/cityrepdata.c on localhost failed
make[3]: *** [cityrepdata.lo] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [all-recursive] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2
------------------------------------------------------------
gcc (Debian 4.7.2-4) 4.7.2

Regards,
Roland

Roland Haeder <quix0r>

 

(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

Attached Files
file #16983:  DatumUnion.patch added by cazfi (2kB - text/x-diff)
file #16947:  config.log added by quix0r (196kB - text/x-log - config.log + config.status)
file #16948:  config.status added by quix0r (106kB - application/octet-stream - config.log + config.status)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by cazfi (Posted a comment)
  • -unavailable- added by jtn (Updated the item)
  • -unavailable- added by quix0r (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Error: not logged in

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue Jan 22 16:42:16 2013cazfiStatusReady For Test=>Fixed
      Assigned toNone=>cazfi
      Open/ClosedOpen=>Closed
    Sun Jan 20 20:56:46 2013cazfiAttached File-=>Added DatumUnion.patch, #16983
      StatusNone=>Ready For Test
      Planned Release=>2.3.4, 2.4.0, 2.5.0
    Tue Jan 15 00:43:53 2013quix0rAttached File-=>Added config.log, #16947
      Attached File-=>Added config.status, #16948
    Wed Jan 9 09:29:51 2013jtnSummary\'d.string_value\' was declared here=>cityrepdata.c: split_string(): 'd.string_value' may be used uninitialized in this function
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup