On Tue, 2009-06-09 at 08:51 +0200, Øyvind Harboe wrote:
> On Tue, Jun 9, 2009 at 8:29 AM, Zach Welch<z...@superlucidity.net> wrote:
> > On Tue, 2009-06-09 at 07:57 +0200, Øyvind Harboe wrote:
> >> How about a clearer policy of using assert()'s?
> >>
> >> I'm thinking that error()'s should be reserved for "real" runtime errors.
> >>
> >>
> >> > if (!tap_is_state_stable(path[num_states - 1]))
> >> > {
> >> > LOG_ERROR("BUG: TAP path doesn't finish in a stable
> >> > state");
> >> > - exit(-1);
> >> > + jtag_set_error(ERROR_JTAG_NOT_STABLE_STATE);
> >> > + return;
> >> > }
> >>
> >>
> >> The above should be an assert().
> >
> > No. I thought this issue over carefully. The problem with using
> > asserts is that the program should not simply exit when the users issues
> > a bad command. That is a _policy_. The core JTAG module should
> > implement as few policies as possible, focusing on _mechanisms_. Thus,
> > I removed the "exit" policy error and used the error reporting mechanism
> > to make the pathmove mechanism work in more contexts.
> >
> > If it will be possible for the user to script a call such that their
> > typos get translated to a bad pathmove call, then they should not
> > suddenly see OpenOCD stop running. Furthermore, compiling with -DNDEBUG
> > _removes_ assertions.... so you want to disable error checking for
> > releases, do you?
>
> I want assertions to be compiled out when:
>
> - they are the type of errors that only occur due to programming errors.
> - they impact performance on embedded targets.
Then we are not talking about assertions. You do not get to define the
terms of assertions, <assert.h> does.
So, we are talking about "debugging checks" that can be compiled out for
performance. Well, there was no performance changed here.
> > By deferring the policy decision to the caller, the _program_ can choose
> > to live or die when a potentially user-supplied parameter was incorrect.
> > This is the right move for creating a reusable JTAG library, as you must
> > assume the user will do things with the core that you did not intend.
>
> The problem I have with this is where do you stop? Do we check if
> pointers are valid?
>
> I like clear programming errors to simply use asserts.
Right, but my point is there is _no way to tell_ if it is a programming
error or a user error in a library. You would be presuming too much.
> > For example, this change allows an enterprising individual to begin
> > writing a TCL replacement of the Target layer, adding low-level pathmove
> > and scan commands for Jim (and so on... ad sanatorium). Moreover, if
> > any command that builds on this function were to be able to be similarly
> > misused, the program should not exit -- the interpreter should report an
> > error, right?
>
> Yes, but the error checks should not necessarily be done at the very lowest
> level when it affects performance of embedded targets.
Unfortunately, this is exactly where they belong in this method. Those
error checks must do validation of the path, which could well be fed to
the routine from a TCL script or other application that takes user
input. Guess what... jtag_add_pathmove is called by jtag_add_statemove
which is called by... SVF which opens this door wide open, doesn't it?
> > P.S. In raising this point, you have exposed the fact that assert has
> > been used several throughout the JTAG module incorrectly. Expect a
> > flurry of patches to fix these bugs this soon.
>
> Don't do that please. At least we should have a way to distinguish
> between what are programming errors (asserts) and what are
> real user/environment errors.
That is exactly the problem though: if we want the JTAG layer of
libopenocd to be general purpose, then we cannot make assumptions about
the source of the errors in this data. Mind you, there are places that
assert is valid, but there are places where it is not:
static void jtag_checks(void)
{
assert(jtag_trst == 0);
}
As far as I can tell, this also needs to be changed to a run-time error,
_unless_ it is never possible for this value to be wrong by the user's
bad actions. If it does, then it would be desirable to abort the
current operation and allow the program to try and gracefully recover,
in the case that the user was silly enough to end up in this state.
Cheers,
Zach
_______________________________________________
Openocd-development mailing list
Openocd-development@lists.berlios.de
https://lists.berlios.de/mailman/listinfo/openocd-development
