This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH v3] testsuite: Treat an empty string in needs_status_wrapper as false


> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> 
> Yeah, this is all odd.
> 
> Looking at gcc's testsuite, even though lots of inconsistency
> had been centralized in wrapper.exp years ago:
> 
>  http://lists.gnu.org/archive/html/dejagnu/2004-09/msg00001.html
> 
> they still remain largely inconsistent:
 [AK:]  Yes I've noticed inconsistency there, GCC test suite should be fixed as well, I think.

> 
> Given all that, I don't think anyone will be setting target_info
> needs_status_wrapper to "0" to say "false".  It just doesn't seem
> it'd work properly anywhere.  I'd guess that "0" check was a little
> mistake that ended blindly copy/pasted everywhere.
[AK:] GCC developer in our project sets needs_status_wrapper to 0. So this happens, even if it is done by mistake. 

> 
> But there's only one that's odd... It looks like:
> 
>  baseboards/mn10300-sim.exp:49:set_board_info needs_status_wrapper ""
> 
> Sigh.  Someone used the empty string to enable the status wrapper...
[AK:] Or someone tried to explicitly disable it.

> 
> Looking above that, to see if there's any comment that explains why
> that one is different, we find:
> 
>  baseboards/mn10300-sim.exp:48:# The simulator doesn't return exit
> statuses and we need to indicate this.
>  baseboards/mn10300-sim.exp-49-set_board_info needs_status_wrapper ""
> 
> Which is just the same comment that appears in all the
> other boards.  So it just looks like nobody tried that
> board with gcc?  I think we can just ignore that.
> 
> In any case, no board set needs_status_wrapper to 0.  I wonder
> if we shouldn't stop looking for "0" explicitly too, like DejaGNU,
> and clean up gcc's checks likewise...
[AK:] In my opinion, there should be legitimate "false" value not just unset value. It shouldn't be just 0, but anything the Tcl treats as false (0, false, no, off). Regrettably, unlike, for example JavaScript, empty string is not a "false" value in Tcl. 

> 
> But how did you notice this?  What value were you setting in
> your board?
[AK:] I've set it to an empty string to disable status wrapper, as it looked like a legitimate way of doing this after studying GCC test suite. It is a long story of how I've reached this, but now I see that this isn't how it is supposed to be done.

> 
> 
> In TCL, like in C, "1" and "0" are the same as true and false.  This
> can just be:
> 
>     if {[gdb_needs_status_wrapper]} {
> 
[AK:]  I was in doubt about how to write this, because there are enough cases of "[]== 1" in test suite, so I've stick to them, as it looks like a more clear way, but I don't have own preferences here, so I will update this for next version.

> 
> But then again, if setting to "0" indeed breaks the DejaGNU
> side (in that it treats that as true), I question the value
> of propagating that brokenness...
[AK:] I would advocate the need to update dejagnu, as it seems that both GCC and GDB consider zero as a false value. As I've said to me it looks better to have some legitimate false value to this property, not only true or unset.

> 
> --
> Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]