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. 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. 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. Regards, Simon -- To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
