[Freeciv-Dev] [bug #22014] road_integrators_cache_init(): conditional jump or move depends on uninitialised

[Emmet Hikory]

Update of bug #22014 (project freeciv):

  Status:  Ready For Test => Fixed  
 Open/Closed:Open => Closed 


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #22014] road_integrators_cache_init(): conditional jump or move depends on uninitialised

[Emmet Hikory]

Update of bug #22014 (project freeciv):

  Status:None => Ready For Test 
 Assigned to:None => persia 
 Planned Release: => 2.6.0  

___

Follow-up Comment #7:

Aha, yes.  Attached is a patch moving it outside that context, which has the
side benefit of only running it once (which ought improve performance). 
Thanks a lot for the help debugging this.



(file #20661)
___

Additional Item Attachment:

File name: move-road-integrator-cache-init-outside-road-loop.patch Size:0 KB


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #22014] road_integrators_cache_init(): conditional jump or move depends on uninitialised

[pepeto]

Follow-up Comment #6, bug #22014 (project freeciv):

> Looking at the caller (in ruleset.c), the bitvector is cleared
> prior to a for loop.

But it is included in a road type iteration, so the integrates filed of roads
listed later is not initialized.

I see a function named road_type_init(). Shouldn't it be initialized there?
Else it could be initialized in another loop in load_rulesetdir()?


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #22014] road_integrators_cache_init(): conditional jump or move depends on uninitialised

[Emmet Hikory]

Follow-up Comment #5, bug #22014 (project freeciv):

Indeed it does.  Using the construction:


  int road_idx = road_index(oroad);
  bv_roads integrates = proad->integrates;
  bool integrator = FALSE;

  /* BV_CLR_ALL(integrates); */
  /* integrator = BV_ISSET(integrates, road_idx); */
  if (integrates.vec[road_idx] == TRUE) {
integrator = TRUE;
  }


results in the same warning on the integrates.vec[] check.  If the BV_CLR_ALL
statement is uncommented, there are no warnings.  Looking at the caller (in
ruleset.c), the bitvector is cleared prior to a for loop.  The for loop
conditionally sets some values with BV_SET().  I'm unsure how this could be
uninitialised, unless there is something odd about the pointer mapping between
extras and roads.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #22014] road_integrators_cache_init(): conditional jump or move depends on uninitialised

[Marko Lindqvist]

Follow-up Comment #4, bug #22014 (project freeciv):

It just means integrator derives uninitialized value.

I assume:
integrator = BV_ISSET(integrates, road_idx) gets uninitialized bit from
"integrates" ->
proad->integrates has the bit unitialized


___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #22014] road_integrators_cache_init(): conditional jump or move depends on uninitialised

[Emmet Hikory]

Follow-up Comment #3, bug #22014 (project freeciv):

Unpacking the relevant code, it seems that even if the value is initialised,
it becomes uninitialised in the BV_ISSET call.  I'm not sure how this function
differs from every other use of BV_ISSET though.

Using the less dense function:


/
  Initialize the road integrators cache
/
void road_integrators_cache_init(void)
{
  road_type_iterate(proad) {
proad->integrators = road_type_list_new();
/* Roads always integrate with themselves. */
road_type_list_append(proad->integrators, proad);
road_type_iterate(oroad) {
  int road_idx = road_index(oroad);
  bv_roads integrates = proad->integrates;
  bool integrator = FALSE;

  integrator = BV_ISSET(integrates, road_idx); 
  if (integrator) {
road_type_list_append(proad->integrators, oroad);
  }
} road_type_iterate_end;
road_type_list_unique(proad->integrators);
road_type_list_sort(proad->integrators, &compare_road_move_cost);
  } road_type_iterate_end;
}


Valgrind now reports the same error for "if (integrator)", where integrator is
known to be initialised prior to the BV_ISSET() macro.

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #22014] road_integrators_cache_init(): conditional jump or move depends on uninitialised

[pepeto]

Follow-up Comment #2, bug #22014 (project freeciv):

Server side, with default ruleset (didn't load anything else).

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #22014] road_integrators_cache_init(): conditional jump or move depends on uninitialised

[Emmet Hikory]

Follow-up Comment #1, bug #22014 (project freeciv):

This function exists only in trunk, so no need to check other branches.  Was
this found in the client or in the server?  Also, which ruleset was loaded?

___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

[Freeciv-Dev] [bug #22014] road_integrators_cache_init(): conditional jump or move depends on uninitialised

[pepeto]

URL:
  

 Summary: road_integrators_cache_init(): conditional jump or
move depends on uninitialised
 Project: Freeciv
Submitted by: pepeto
Submitted on: Mon 05 May 2014 10:53:46 PM CEST
Category: None
Severity: 3 - Normal
Priority: 5 - Normal
  Status: None
 Assigned to: None
Originator Email: 
 Open/Closed: Open
 Release: trunk r24854
 Discussion Lock: Any
Operating System: None
 Planned Release: 

___

Details:

Noticed in valgrind output for trunk (I didn't check other branches):

==3256== Conditional jump or move depends on uninitialised value(s)
==3256==at 0x62D37A: road_integrators_cache_init (road.c:135)
==3256==by 0x4CAE37: load_rulesetdir (ruleset.c:3298)
==3256==by 0x4CC3DC: load_rulesets (ruleset.c:6344)
==3256==by 0x43A2EA: srv_main (srv_main.c:2528)
==3256==by 0x432681: main (civserver.c:454)






___

Reply to this item at:

  

___
  Message sent via/by Gna!
  http://gna.org/


___
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev