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 09 Jan 2013 04:43:45 AM UTC  
 
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.0

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 22 Jan 2013 04:42:04 PM UTC, 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 22 Jan 2013 04:42:00 PM UTC, 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 22 Jan 2013 04:41:22 PM UTC, 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 20 Jan 2013 08:56:46 PM UTC, 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 19 Jan 2013 09:16:24 AM UTC, 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 19 Jan 2013 07:59:14 AM UTC, 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 15 Jan 2013 12:44:56 AM UTC, comment #3:

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

Roland Haeder <quix0r>
Mon 14 Jan 2013 11:47:56 PM UTC, 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 14 Jan 2013 08:14:38 AM UTC, 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 09 Jan 2013 04:43:45 AM UTC, 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.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 9 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Tue 22 Jan 2013 04:42:16 PM UTCcazfiStatusReady For Test=>Fixed
      Assigned toNone=>cazfi
      Open/ClosedOpen=>Closed
    Sun 20 Jan 2013 08:56:46 PM UTCcazfiAttached File-=>Added DatumUnion.patch, #16983
      StatusNone=>Ready For Test
      Planned Release=>2.3.4, 2.4.0, 2.5.0
    Tue 15 Jan 2013 12:43:53 AM UTCquix0rAttached File-=>Added config.log, #16947
      Attached File-=>Added config.status, #16948
    Wed 09 Jan 2013 09:29:51 AM UTCjtnSummary\'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