On Sun, Sep 25, 2011 at 4:04 AM, David Gibson
<da...@gibson.dropbear.id.au> wrote:
> On Fri, Sep 23, 2011 at 11:02:54AM -0700, Anton Staaf wrote:
>> Cells of size 8, 16, 32, and 64 bits are supported. The new
>> /bits/ syntax was selected so as to not pollute the reserved
>> keyword space with uint8/uint16/... type names.
>>
>> With this patch the following property assignment:
>>
>> property = /bits/ 16 <0x1234 0x5678 0x0 0xffff>;
>>
>> is equivalent to:
>>
>> property = <0x12345678 0x0000ffff>;
>>
>> It is now also possible to directly specify a 64 bit literal in a
>> cell list using:
>>
>> property = /bits/ 64 <0xdeadbeef00000000>;
>>
>> It is an error to attempt to store a literal into a cell that is too
>> small to hold the literal, and the compiler will generate an error
>> when it detects this. For instance:
>>
>> property = /bits/ 8 <256>;
>>
>> Will fail to compile. It is also an error to attempt to place a
>> reference in a non 32-bit cell.
>
> So, there's one small but serious error left in the patch, see below.
>
> Other than that, only two things concern me:
>
> 1) Exactly what to call the keyword. /bits/ is better than
> /size/, but I'd still like to stew a bit on it to see if we can come
> up with anything better.
>
> 2) In the documentation and internal naming, we're using
> the term "cell" ambiguously. In the discussion of this extension
> we've been using "cell" to mean one array element, which is fair
> enough since it's more-or-less standard CS terminology. However, in
> previous FDT and OF terminology "cell" refers to an exactly 32-bit
> quantity [well, to be exact OF old-timers say this isn't strictly
> right in old OF-ese, but it's close enough to meaning that for most
> purposes].
> So, I think we need to find a different name for array cells
> to clarify the terminology. "elements" maybe?
>
> [snip]
>> + | celllistprefix DT_REF
>> {
>> - $$ = eval_literal($1, 0, 32);
>> + uint64_t val = ~0ULL >> (64 - $1.bits);
>> +
>> + if ($1.bits != 32)
>> + print_error("References only allowed in 32-bit"
>> + " cell lists");
>> +
>> + $$.data = data_add_marker($1.data, REF_PHANDLE, $2);
>> + $$.data = data_append_integer($$.data, val, $1.bits);
>
> Uh, so, this is actually worse than what you had before. Remember
> print_error() just prints an error, without aborting the parse. So
> now, if bits != 32 it will print the error, then add a phandle marker
> *and* a bits-sized -1. So later, when we go through and fix up the
> REF_PHANDLE markers, we'll assume we're replacing 32-bits of data,
> which could overrun the end of the data if the element size is less
> than 32.
>
> So, basically the data_add_marker() needs to be in an else clause.
Yup, you're right. I should first ask, I couldn't figure out how to write
a test that tests parse error cases like this. The closest I found was the
check tests. But they are specific to the check messages. Is there a
good example of a parse error test? Or should I create a new type of test?
I'll fix this up today.
Thanks,
Anton
> --
> 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
>
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
