[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Indu Bhagat
Subject: Re: [PATCH] pickles: ctf: fix issues, support CTF slices
Date: Tue, 16 Feb 2021 22:35:43 -0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1

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 =
@@ -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_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 :)


Ok. I will do this for the next commits.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]