Re: [PATCH] pickles: ctf: fix issues, support CTF slices

[Jose E. Marchesi]

Hi Indu!

> On 2/16/21 10:38 AM, Jose E. Marchesi wrote:
>>> +type CTF_Slice =
>>> +  struct
>>> +  {
>>> +    CTF_Type_Id cts_type;
>>> +    uint<16> cts_offset;
>>> +    uint<16> cts_bits;
>>> +  };
>> I wonder if it would make sense for cts_offset and cts_bits to be
>> offsets (in bits)?
> Hmm..it does. It makes the unit explicit to bits. I have changed to
> this type in the next version of the patch.
>>> +
>>>   type CTF_Type =
>>>     struct
>>>     {
>>> @@ -257,9 +265,14 @@ type CTF_Type =
>>>         CTF_Integer_Type integer         : info.kind == CTF_KIND_INTEGER;
>>>         CTF_Float_Type float             : info.kind == CTF_KIND_FLOAT;
>>>         CTF_Array array                  : info.kind == CTF_KIND_ARRAY;
>>> +      CTF_Slice slice                     : info.kind == CTF_KIND_SLICE;
>> Careful with tabs there.  Please use spaces instead.
>> Same in other parts of the patch.
> Sorry about this. I see GNU Poke HACKING file recommends no tabs.
>>>         CTF_Member[info.vlen] members    : info.kind in 
>>> [CTF_KIND_STRUCT,CTF_KIND_UNION];
>>> -      CTF_Enum enum                    : info.kind == CTF_KIND_ENUM;
>>> -      CTF_Type_Id[info.vlen] arg_types : info.kind == CTF_KIND_FUNCTION;
>>> +      CTF_Enum[info.vlen] enum            : info.kind == CTF_KIND_ENUM;
>>> +      struct
>>> +      {
>>> +   CTF_Type_Id[info.vlen] arg_types : info.kind == CTF_KIND_FUNCTION;
>>> +   uint32[info.vlen % 2] padding;
>>> +      } func_args;
>> To be more coherent with the current code, I would suggest to use a
>> separated type CTF_Func_Args instead of an anonymous struct there.
>>
>> Also, I think it makes more sense to put the constraint expression in
>> func_args instead of arg_types: less coupling.
>>
>> CTF_Func_Args func_args : info.kind == CTF_KIND_FUNCTION;
> Ok. I moved the definition into CTF_Type (as the token info.vlen needs
> to available).

Perfect!

>>
>>>         struct {} nothing;
>>>       } data;
>>>     };
>>> @@ -268,10 +281,15 @@ fun ctf_string = (Elf64_File elf,
>>>                     CTF_Header header,
>>>                     CTF_Name name) string:
>>>   {
>>> - if (name.stid == CTF_STRTAB_0)
>>> -   return string @ (header.cth_stroff + name.offset);
>>> - else
>>> -   return elf.get_string (name.offset);
>>> +  if (name.stid == CTF_STRTAB_0)
>>> +  {
>>> +    assert (name.offset < header.cth_strlen,
>>> +       "Invalid string offset into CTF strtab");
>> Hmm, I would probably raise E_inval instead of doing an assert in that
>> condition: it is actually perfectly possible to pass an invalid offset
>> to `ctf_string'.
>>
>>> +    return string @ (header'offset + header'size + header.cth_stroff
>>> +                + name.offset);
>>> +  }
>>> +  else
>>> +    return elf.get_string (name.offset);
>>>   }
>>>     fun ctf_get_header = (Elf64_File elf) CTF_Header:
>> If you create a user in savannah.gnu.org I will give you write
>> permissions to the repo so you can check-in your own patches.
>>
>> Just let me know the username you chose in savannah :)
>>
>> Thanks!
>
> Ok. I will do this for the next commits.

Ok.