On Tue, Jun 12, 2018 at 09:04:21PM -0600, Simon Glass wrote: > Hi David, > > On 11 June 2018 at 22:42, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: > > On Sat, Jun 09, 2018 at 11:12:44AM -0800, Simon Glass wrote: > >> Hi David, > >> > >> On 8 June 2018 at 04:27, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: > >> > On Wed, Jun 06, 2018 at 03:37:02PM -0600, Simon Glass wrote: > >> >> Export all of these through Python. > >> >> > >> >> Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx> > >> >> --- > >> >> > >> >> Changes in v2: > >> >> - Drop use of check_err() since these functions cannot fail > >> >> - Update existing header functions to also drop check_err() > >> >> > >> >> pylibfdt/libfdt.i | 71 +++++++++++++++++++++++++++++++++++++++-- > >> >> tests/pylibfdt_tests.py | 8 +++++ > >> >> 2 files changed, 76 insertions(+), 3 deletions(-) > >> >> > >> >> diff --git a/pylibfdt/libfdt.i b/pylibfdt/libfdt.i > >> >> index 94c3d00..f33e2ab 100644 > >> >> --- a/pylibfdt/libfdt.i > >> >> +++ b/pylibfdt/libfdt.i > >> >> @@ -252,21 +252,86 @@ class Fdt: > >> >> """ > >> >> return check_err(fdt_next_subnode(self._fdt, nodeoffset), quiet) > >> >> > >> >> + def magic(self): > >> >> + """Return the magic word from the header > >> >> + > >> >> + Returns: > >> >> + Magic word > >> >> + """ > >> >> + # Use a mask to ensure that this does not return a -ve number > >> >> + return fdt_magic(self._fdt) & 0xffffffff > >> > > >> > You still have the mask here for no clear reason. > >> > >> Python's numbers are a little strange. If the top bit of a 32-bit > >> number is set, this means it is a negative number in two-complement > >> arithmetic, as you know. Python will then use a negative value instead > >> of positive. To force it to regard the number as unsigned, we must > >> mask it. This is how I have learned to do it in Python, but maybe > >> there is a better way? > > > > Two observations first: > > > > 1) I'm pretty sure this will only apply on 32-bit platforms > > (specifically those where a C "long" and therefore a Python int() are > > 32-bit). > > > > 2) I think this has more to do with the swig typemapping that the > > integers in Python per se. On a 32-bit platform I can do this in > > Python: > > >>> (1 << 31) > > 2147483648L > > > > So we're getting the expected answer... but as a Python long rather > > than a Python int (it's a Python int when run on a 64-bit platform). > > > > So what seems to be happening here is that on the C side (not sure if > > it's in stuff you've written or in swig's generated magic), we're > > forcing the 'uint32_t' into a 'long', then stuffing that into a Python > > integer. > > > > The mask strips away the sign, and at the same time coerces the value > > into a Python long (because 0xffffffff will be a PyLong on such a > > platform). > > > >> To see the impact, try removing the mask. You will see that magic() > >> will return a -ve number, rather than unsigned. > > > > So, the thing that bothers me a bit here is that it's not the same for > > all the header functions, even though they all return uint32_t on the > > C side. I guess we get away with in practice, because apart from > > 'magic' all the other values should actually fit in 31 bits. Could > > still cause confusing answers on a malformed fdt, of course. > > > > I think the right solution is to alter the swig stuff so that on > > platforms where sizeof(long) <= 4 we make the header functions return > > a PyLong instead of a PyInt. I'm not immediately sure how to do that, > > however. > > > > I guess the current approach will work for now, even if it is a hack, > > so I'll go ahead and apply it, and we can try to fix it up later. > > Thanks for digging into this. Yes this is definitely a typemap thing > and I think I have a culprit. > > The #defines in libfdt.h are actually not used with swig, since it > doesn't support that. Instead I am re-declaring these functions, and > when I do so, they are declared as int. They should be fdt32_t to > match the struct member types. Actually, they should be uint32_t. fdt32_t specifically refers to a 32-bit integer *in big endian representation. > On top of that the fdt32_t type in the header is declared as int. This > type has so far only been used for struct members, e.g. struct > fdt_property so it didn't matter. But really it should be uint32_t to > match the libfdt.h header. Yes, that needs fixing. > That would make everything consistent I think. A quick test suggests > it solves the problem although I need to convince myself that other > changes aren't also needed. After that I'll send a patch to modify all > of those. Great. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
