Re: [libunwind] x86-64 patch for libunwind v0.97

[David Mosberger]

Hi Max,

Here is some feedback on the x86-64 patch for libunwind:

Could you split the patch into DWARF-fixes and x86-64-enablement?

 - libunwind-x86_64.h:

    o For x86_64_regnum_t, it would make sense to reserve a sufficient
      number of registers asap.  Adding stuff here is effectively a
      binary-interface-change so it's better to reserve enough
      register numbers earlier on.  The FP stuff is hairy though on
      x86.  I hope the stuff in x86_regnum_t is fairly complete, but
      I'm not really enough of an x86-expert to know for sure.

    o UNW_TDEP_NUM_EH_REGS: This is the number of exception-handling
      argument registers.  On x86, this is 2; on ia64, it's 4.  I'm not
      sure what x86-64 uses.  GCC macro EH_RETURN_DATA_REGNO() should
      tell you, though.  Note that the registers used for passing exception
      data MUST be numbered consecutively in x86_64_regnum_t.  This is
      why x86_regnum_t lists EAX and EDX next to each other.

 - Gfind_proc_info-lsb.c:

    o This looks wrong to me:

-    unw_word_t start_ip_offset;
-    unw_word_t fde_offset;
+    int32_t start_ip;
+    int32_t fde_addr;

      DWARF3 allows for text > 4GB and I'd like to avoid introducing
      artificial 32-bit limits into libunwind.  There are several
      related changes in Gfind_proc_info-lsb.c which probably
      shouldn't be there either.  Perhaps the real issue is
      signedness?

    o Actually, I don't understand the motivation for most of the
      changes to this file.  Can you break them up into individual
      patches so we can discuss them group by group?

  - Gparser-dwarf.c:

    o Such things:

-      Debug (1, "Invalid register number %u\n", *valp);
+      Debug (1, "Invalid register number %lu\n", *valp);

      should be written as:

+      Debug (1, "Invalid register number %lu\n", (unsigned long) *valp);

      That way, they'll compile without warning on all platforms.

    o I'd prefer this:

 static int
+create_state_record_for (struct dwarf_cursor *c, dwarf_state_record_t *sr,
+                        unw_word_t ip);

      to be formatted as:

 static int create_state_record_for (...);

      so that there is a visual clue that we're dealing with a (forward)
      declaration, rather than a definition.

    o This one:

+      /* XXX: this is a workaround */
+      if ((rs->reg[DWARF_CFA_REG_COLUMN].val == UNW_TDEP_SP)
+           && (rs->reg[UNW_TDEP_SP].where == DWARF_WHERE_SAME))
+        cfa = c->cfa;
+

      probably deserves a more detailed comment.  What does it workaround?
      Why is it "correct", etc.?

 - _UPT_reg_offset.c:

     Please don't do this:

-# error Fix me.
+//# error Fix me.

     If you want to change it to a warning, that'd be fine, though.

The rest looks great to me!

Thanks,

        --david