Hi David, On 12 June 2018 at 21:31, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote: > 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. Ah OK, I suppose I knew that. But I think that may the inspiration for my error - just looking at the header struct and assuming that the macros returns the same type. > >> 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. 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
