| Message ID | 20191003041909.23187-10-amitay@ozlabs.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add system device tree to libpdbg | expand |
Just a quick question. Can we assume: target_is_virtual(target_to_virtual(target)) will always be true? And that: target_is_virtual(target_to_real(target)) will always be false? Do you think it be would be worthwhile adding some asserts to check this or does the probing code (which I'm yet to read) somehow guarantee those assertions are always correct? - Alistair On Thursday, 3 October 2019 2:18:48 PM AEDT Amitay Isaacs wrote: > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > --- > libpdbg/target.c | 8 ++++++++ > libpdbg/target.h | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/libpdbg/target.c b/libpdbg/target.c > index 20f292f..f3d4db8 100644 > --- a/libpdbg/target.c > +++ b/libpdbg/target.c > @@ -481,3 +481,11 @@ struct pdbg_target *target_to_real(struct pdbg_target *target) > > return target; > } > + > +struct pdbg_target *target_to_virtual(struct pdbg_target *target) > +{ > + if (target->compatible && target->vnode) > + return target->vnode; > + > + return target; > +} > diff --git a/libpdbg/target.h b/libpdbg/target.h > index 5d04117..8148f83 100644 > --- a/libpdbg/target.h > +++ b/libpdbg/target.h > @@ -66,5 +66,6 @@ const char *pdbg_get_backend_option(void); > struct sbefifo *pib_to_sbefifo(struct pdbg_target *target); > bool target_is_virtual(struct pdbg_target *target); > struct pdbg_target *target_to_real(struct pdbg_target *target); > +struct pdbg_target *target_to_virtual(struct pdbg_target *target); > > #endif >
On Wed, 2019-10-09 at 13:33 +1100, Alistair Popple wrote: > Just a quick question. Can we assume: > > target_is_virtual(target_to_virtual(target)) will always be true? No. That's not strictly correct. target_to_virtual() will only map to virtual targets, if there is a linked virtual target. Otherwise it will return the same target, as there is nothing to map. > And that: > > target_is_virtual(target_to_real(target)) will always be false? Similar argument as above. Some virtual nodes may not have any linked real nodes. In that case, target_to_real() will return the same virtual target. (For example, "/proc0" is a virtual target without any real target linked to it.) > > Do you think it be would be worthwhile adding some asserts to check > this or > does the probing code (which I'm yet to read) somehow guarantee > those > assertions are always correct? I think the tests which focus on the traverse will ensure the correctness of map functions. We don't specifically need testing for the lower level functions. Also, these functions are internal to libpdbg and I would have to jump through hoops to test them. :-) But not impossible to add tests if you strongly feel about it. > > - Alistair > > On Thursday, 3 October 2019 2:18:48 PM AEDT Amitay Isaacs wrote: > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > > --- > > libpdbg/target.c | 8 ++++++++ > > libpdbg/target.h | 1 + > > 2 files changed, 9 insertions(+) > > > > diff --git a/libpdbg/target.c b/libpdbg/target.c > > index 20f292f..f3d4db8 100644 > > --- a/libpdbg/target.c > > +++ b/libpdbg/target.c > > @@ -481,3 +481,11 @@ struct pdbg_target *target_to_real(struct > > pdbg_target > *target) > > > > return target; > > } > > + > > +struct pdbg_target *target_to_virtual(struct pdbg_target *target) > > +{ > > + if (target->compatible && target->vnode) > > + return target->vnode; > > + > > + return target; > > +} > > diff --git a/libpdbg/target.h b/libpdbg/target.h > > index 5d04117..8148f83 100644 > > --- a/libpdbg/target.h > > +++ b/libpdbg/target.h > > @@ -66,5 +66,6 @@ const char *pdbg_get_backend_option(void); > > struct sbefifo *pib_to_sbefifo(struct pdbg_target *target); > > bool target_is_virtual(struct pdbg_target *target); > > struct pdbg_target *target_to_real(struct pdbg_target *target); > > +struct pdbg_target *target_to_virtual(struct pdbg_target *target); > > > > #endif > > > > > Amitay.
On Wednesday, 9 October 2019 1:40:20 PM AEDT Amitay Isaacs wrote: > On Wed, 2019-10-09 at 13:33 +1100, Alistair Popple wrote: > > Just a quick question. Can we assume: > > > > target_is_virtual(target_to_virtual(target)) will always be true? > > No. That's not strictly correct. target_to_virtual() will only map to > virtual targets, if there is a linked virtual target. Otherwise it > will return the same target, as there is nothing to map. > > > > And that: > > > > target_is_virtual(target_to_real(target)) will always be false? > > Similar argument as above. Some virtual nodes may not have any linked > real nodes. In that case, target_to_real() will return the same > virtual target. (For example, "/proc0" is a virtual target without any > real target linked to it.) > So in these cases are we asserting that if we fail to map a node to a [virtual|real] node that it is still correct to continue on treating the original node as if it were of the requested type? > > Do you think it be would be worthwhile adding some asserts to check > > this or > > does the probing code (which I'm yet to read) somehow guarantee > > those > > assertions are always correct? > > I think the tests which focus on the traverse will ensure the > correctness of map functions. We don't specifically need testing for > the lower level functions. Also, these functions are internal to > libpdbg and I would have to jump through hoops to test them. :-) But > not impossible to add tests if you strongly feel about it. It wasn't so much for testing but rather to ensure we crash and burn as early as possible if something unexpected (ie. buggy) is observed. However it seems my original assertions were wrong anyway. - Alistair > > > > - Alistair > > > > On Thursday, 3 October 2019 2:18:48 PM AEDT Amitay Isaacs wrote: > > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > > > --- > > > libpdbg/target.c | 8 ++++++++ > > > libpdbg/target.h | 1 + > > > 2 files changed, 9 insertions(+) > > > > > > diff --git a/libpdbg/target.c b/libpdbg/target.c > > > index 20f292f..f3d4db8 100644 > > > --- a/libpdbg/target.c > > > +++ b/libpdbg/target.c > > > @@ -481,3 +481,11 @@ struct pdbg_target *target_to_real(struct > > > pdbg_target > > *target) > > > > > > return target; > > > } > > > + > > > +struct pdbg_target *target_to_virtual(struct pdbg_target *target) > > > +{ > > > + if (target->compatible && target->vnode) > > > + return target->vnode; > > > + > > > + return target; > > > +} > > > diff --git a/libpdbg/target.h b/libpdbg/target.h > > > index 5d04117..8148f83 100644 > > > --- a/libpdbg/target.h > > > +++ b/libpdbg/target.h > > > @@ -66,5 +66,6 @@ const char *pdbg_get_backend_option(void); > > > struct sbefifo *pib_to_sbefifo(struct pdbg_target *target); > > > bool target_is_virtual(struct pdbg_target *target); > > > struct pdbg_target *target_to_real(struct pdbg_target *target); > > > +struct pdbg_target *target_to_virtual(struct pdbg_target *target); > > > > > > #endif > > > > > > > > > > > Amitay. >
On Wed, 2019-10-09 at 13:53 +1100, Alistair Popple wrote: > On Wednesday, 9 October 2019 1:40:20 PM AEDT Amitay Isaacs wrote: > > On Wed, 2019-10-09 at 13:33 +1100, Alistair Popple wrote: > > > Just a quick question. Can we assume: > > > > > > target_is_virtual(target_to_virtual(target)) will always be true? > > > > No. That's not strictly correct. target_to_virtual() will only map > > to > > virtual targets, if there is a linked virtual target. Otherwise it > > will return the same target, as there is nothing to map. > > > > > > > And that: > > > > > > target_is_virtual(target_to_real(target)) will always be false? > > > > Similar argument as above. Some virtual nodes may not have any > > linked > > real nodes. In that case, target_to_real() will return the same > > virtual target. (For example, "/proc0" is a virtual target without > > any > > real target linked to it.) > > > > So in these cases are we asserting that if we fail to map a node to > a > [virtual|real] node that it is still correct to continue on treating > the > original node as if it were of the requested type? Well the motivation behind target_to_real() and target_to_virtual() is adding convinience functions that help improve the readability of the traverse code where they are used. As I described they are not strictly converting to real/virtual targets, but if possible map it to the right target. I can add some comments for these functions if that would help. > > > > Do you think it be would be worthwhile adding some asserts to > > > check > > > this or > > > does the probing code (which I'm yet to read) somehow guarantee > > > those > > > assertions are always correct? > > > > I think the tests which focus on the traverse will ensure the > > correctness of map functions. We don't specifically need testing > > for > > the lower level functions. Also, these functions are internal to > > libpdbg and I would have to jump through hoops to test them. :- > > ) But > > not impossible to add tests if you strongly feel about it. > > It wasn't so much for testing but rather to ensure we crash and burn > as early > as possible if something unexpected (ie. buggy) is observed. However > it seems > my original assertions were wrong anyway. I have added assertions where we expect a virtual target. > > - Alistair > > > > - Alistair > > > > > > On Thursday, 3 October 2019 2:18:48 PM AEDT Amitay Isaacs wrote: > > > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > > > > --- > > > > libpdbg/target.c | 8 ++++++++ > > > > libpdbg/target.h | 1 + > > > > 2 files changed, 9 insertions(+) > > > > > > > > diff --git a/libpdbg/target.c b/libpdbg/target.c > > > > index 20f292f..f3d4db8 100644 > > > > --- a/libpdbg/target.c > > > > +++ b/libpdbg/target.c > > > > @@ -481,3 +481,11 @@ struct pdbg_target *target_to_real(struct > > > > pdbg_target > > > *target) > > > > > > > > return target; > > > > } > > > > + > > > > +struct pdbg_target *target_to_virtual(struct pdbg_target > > > > *target) > > > > +{ > > > > + if (target->compatible && target->vnode) > > > > + return target->vnode; > > > > + > > > > + return target; > > > > +} > > > > diff --git a/libpdbg/target.h b/libpdbg/target.h > > > > index 5d04117..8148f83 100644 > > > > --- a/libpdbg/target.h > > > > +++ b/libpdbg/target.h > > > > @@ -66,5 +66,6 @@ const char *pdbg_get_backend_option(void); > > > > struct sbefifo *pib_to_sbefifo(struct pdbg_target *target); > > > > bool target_is_virtual(struct pdbg_target *target); > > > > struct pdbg_target *target_to_real(struct pdbg_target > > > > *target); > > > > +struct pdbg_target *target_to_virtual(struct pdbg_target > > > > *target); > > > > > > > > #endif > > > > > > > > > > > > > > Amitay. > > > > > Amitay.
> > > > > > Similar argument as above. Some virtual nodes may not have any > > > linked > > > real nodes. In that case, target_to_real() will return the same > > > virtual target. (For example, "/proc0" is a virtual target without > > > any > > > real target linked to it.) > > > > > > > So in these cases are we asserting that if we fail to map a node to > > a > > [virtual|real] node that it is still correct to continue on treating > > the > > original node as if it were of the requested type? > > > Well the motivation behind target_to_real() and target_to_virtual() is > adding convinience functions that help improve the readability of the > traverse code where they are used. As I described they are not > strictly converting to real/virtual targets, but if possible map it to > the right target. > > I can add some comments for these functions if that would help. Yeah, I think some comments pointing this out would be helpful as in six months time at least I will have forgotten this when it comes time to use them :-) > > > > > > Do you think it be would be worthwhile adding some asserts to > > > > check > > > > this or > > > > does the probing code (which I'm yet to read) somehow guarantee > > > > those > > > > assertions are always correct? > > > > > > I think the tests which focus on the traverse will ensure the > > > correctness of map functions. We don't specifically need testing > > > for > > > the lower level functions. Also, these functions are internal to > > > libpdbg and I would have to jump through hoops to test them. :- > > > ) But > > > not impossible to add tests if you strongly feel about it. > > > > It wasn't so much for testing but rather to ensure we crash and burn > > as early > > as possible if something unexpected (ie. buggy) is observed. However > > it seems > > my original assertions were wrong anyway. > > I have added assertions where we expect a virtual target. > > > > - Alistair > > > > > > - Alistair > > > > > > > > On Thursday, 3 October 2019 2:18:48 PM AEDT Amitay Isaacs wrote: > > > > > Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> > > > > > --- > > > > > libpdbg/target.c | 8 ++++++++ > > > > > libpdbg/target.h | 1 + > > > > > 2 files changed, 9 insertions(+) > > > > > > > > > > diff --git a/libpdbg/target.c b/libpdbg/target.c > > > > > index 20f292f..f3d4db8 100644 > > > > > --- a/libpdbg/target.c > > > > > +++ b/libpdbg/target.c > > > > > @@ -481,3 +481,11 @@ struct pdbg_target *target_to_real(struct > > > > > pdbg_target > > > > *target) > > > > > > > > > > return target; > > > > > } > > > > > + > > > > > +struct pdbg_target *target_to_virtual(struct pdbg_target > > > > > *target) > > > > > +{ > > > > > + if (target->compatible && target->vnode) > > > > > + return target->vnode; > > > > > + > > > > > + return target; > > > > > +} > > > > > diff --git a/libpdbg/target.h b/libpdbg/target.h > > > > > index 5d04117..8148f83 100644 > > > > > --- a/libpdbg/target.h > > > > > +++ b/libpdbg/target.h > > > > > @@ -66,5 +66,6 @@ const char *pdbg_get_backend_option(void); > > > > > struct sbefifo *pib_to_sbefifo(struct pdbg_target *target); > > > > > bool target_is_virtual(struct pdbg_target *target); > > > > > struct pdbg_target *target_to_real(struct pdbg_target > > > > > *target); > > > > > +struct pdbg_target *target_to_virtual(struct pdbg_target > > > > > *target); > > > > > > > > > > #endif > > > > > > > > > > > > > > > > > > > Amitay. > > > > > > > > > > > Amitay. >
diff --git a/libpdbg/target.c b/libpdbg/target.c index 20f292f..f3d4db8 100644 --- a/libpdbg/target.c +++ b/libpdbg/target.c @@ -481,3 +481,11 @@ struct pdbg_target *target_to_real(struct pdbg_target *target) return target; } + +struct pdbg_target *target_to_virtual(struct pdbg_target *target) +{ + if (target->compatible && target->vnode) + return target->vnode; + + return target; +} diff --git a/libpdbg/target.h b/libpdbg/target.h index 5d04117..8148f83 100644 --- a/libpdbg/target.h +++ b/libpdbg/target.h @@ -66,5 +66,6 @@ const char *pdbg_get_backend_option(void); struct sbefifo *pib_to_sbefifo(struct pdbg_target *target); bool target_is_virtual(struct pdbg_target *target); struct pdbg_target *target_to_real(struct pdbg_target *target); +struct pdbg_target *target_to_virtual(struct pdbg_target *target); #endif
Signed-off-by: Amitay Isaacs <amitay@ozlabs.org> --- libpdbg/target.c | 8 ++++++++ libpdbg/target.h | 1 + 2 files changed, 9 insertions(+)