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

[Indu Bhagat]

Thanks for your feedback Jose.

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).
> >         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.

Indu