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

[Jose E. Marchesi]

Hi Indu.
Thanks for the patch.

> +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)?

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

>        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;

>        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!