Change calling convention of dwarf_getmacros to allow opcode 0xff

We now require callers to pass DWARF_GETMACROS_START to start the
iteration.  0 is still accepted, but signals to libdw that the
iteration request comes from an old-style caller, and that opcode 0xff
should be rejected when iterating .debug_macro, to avoid confusion.

Signed-off-by: Petr Machata <pmachata@redhat.com>

9 files changed, 318 insertions, 93 deletions

diff --git a/NEWS b/NEWS
index 8f0780b3..a7ead0a7 100644
--- a/NEWS
+++ b/NEWS
@@ -8,8 +8,6 @@ libdw: New function dwarf_peel_type. dwarf_aggregate_size now uses
        dwarf_macro_getparamcnt, and dwarf_macro_param are available
        for more generalized inspection of macros and their parameters.
 
-XXX: Fix serving of 0xff based on what ends up in Dwarf 5.
-
 Version 0.160
 
 libdw: New functions dwarf_cu_getdwarf, dwarf_cu_die.
diff --git a/libdw/ChangeLog b/libdw/ChangeLog
index bab02e59..909fdbc1 100644
--- a/libdw/ChangeLog
+++ b/libdw/ChangeLog
@@ -1,3 +1,10 @@
+2014-12-02  Petr Machata  <pmachata@redhat.com>
+
+	* dwarf_getmacros.c (token_from_offset, offset_from_token): New
+	helper functions.
+	(do_dwarf_getmacros_die): Merge into dwarf_getmacros.
+	* libdw.h (DWARF_GETMACROS_START): New macro.
+
 2014-11-27  Mark Wielaard  <mjw@redhat.com>
 
 	* Makefile.am (libdw.so): Use textrel_check.
diff --git a/libdw/dwarf_getmacros.c b/libdw/dwarf_getmacros.c
index 8d9d78be..bd64d60f 100644
--- a/libdw/dwarf_getmacros.c
+++ b/libdw/dwarf_getmacros.c
@@ -384,13 +384,57 @@ read_macros (Dwarf *dbg, int sec_index,
   return 0;
 }
 
+/* Token layout:
+
+   - The highest bit is used for distinguishing between callers that
+     know that opcode 0xff may have one of two incompatible meanings.
+     The mask that we use for selecting this bit is
+     DWARF_GETMACROS_START.
+
+   - The rest of the token (31 or 63 bits) encodes address inside the
+     macro unit.
+
+   Besides, token value of 0 signals end of iteration and -1 is
+   reserved for signaling errors.  That means it's impossible to
+   represent maximum offset of a .debug_macro unit to new-style
+   callers (which in practice decreases the permissible macro unit
+   size by another 1 byte).  */
+
+static ptrdiff_t
+token_from_offset (ptrdiff_t offset, bool accept_0xff)
+{
+  if (offset == -1 || offset == 0)
+    return offset;
+
+  /* Make sure the offset didn't overflow into the flag bit.  */
+  if ((offset & DWARF_GETMACROS_START) != 0)
+    {
+      __libdw_seterrno (DWARF_E_TOO_BIG);
+      return -1;
+    }
+
+  if (accept_0xff)
+    offset |= DWARF_GETMACROS_START;
+
+  return offset;
+}
+
+static ptrdiff_t
+offset_from_token (ptrdiff_t token, bool *accept_0xffp)
+{
+  *accept_0xffp = (token & DWARF_GETMACROS_START) != 0;
+  token &= ~DWARF_GETMACROS_START;
+
+  return token;
+}
+
 static ptrdiff_t
 gnu_macros_getmacros_off (Dwarf *dbg, Dwarf_Off macoff,
 			  int (*callback) (Dwarf_Macro *, void *),
-			  void *arg, ptrdiff_t token, bool accept_0xff,
+			  void *arg, ptrdiff_t offset, bool accept_0xff,
 			  Dwarf_Die *cudie)
 {
-  assert (token <= 0);
+  assert (offset >= 0);
 
   if (macoff >= dbg->sectiondata[IDX_debug_macro]->d_size)
     {
@@ -398,23 +442,19 @@ gnu_macros_getmacros_off (Dwarf *dbg, Dwarf_Off macoff,
       return -1;
     }
 
-  ptrdiff_t ret = read_macros (dbg, IDX_debug_macro, macoff,
-			       callback, arg, -token, accept_0xff, cudie);
-  if (ret == -1)
-    return -1;
-  else
-    return -ret;
+  return read_macros (dbg, IDX_debug_macro, macoff,
+		      callback, arg, offset, accept_0xff, cudie);
 }
 
 static ptrdiff_t
 macro_info_getmacros_off (Dwarf *dbg, Dwarf_Off macoff,
 			  int (*callback) (Dwarf_Macro *, void *),
-			  void *arg, ptrdiff_t token, Dwarf_Die *cudie)
+			  void *arg, ptrdiff_t offset, Dwarf_Die *cudie)
 {
-  assert (token >= 0);
+  assert (offset >= 0);
 
   return read_macros (dbg, IDX_debug_macinfo, macoff,
-		      callback, arg, token, true, cudie);
+		      callback, arg, offset, true, cudie);
 }
 
 ptrdiff_t
@@ -428,21 +468,22 @@ dwarf_getmacros_off (Dwarf *dbg, Dwarf_Off macoff,
       return -1;
     }
 
-  /* We use token values > 0 for iteration through .debug_macinfo and
-     values < 0 for iteration through .debug_macro.  Return value of
-     -1 also signifies an error, but that's fine, because .debug_macro
-     always contains at least three bytes of headers and after
-     iterating one opcode, we should never see anything above -4.  */
-  assert (token <= 0);
+  bool accept_0xff;
+  ptrdiff_t offset = offset_from_token (token, &accept_0xff);
+  assert (accept_0xff);
 
-  return gnu_macros_getmacros_off (dbg, macoff, callback, arg, token, true,
-				   NULL);
+  offset = gnu_macros_getmacros_off (dbg, macoff, callback, arg, offset,
+				     accept_0xff, NULL);
+
+  return token_from_offset (offset, accept_0xff);
 }
 
-static ptrdiff_t
-do_dwarf_getmacros_die (Dwarf_Die *cudie, Dwarf_Off *macoffp,
-			int (*callback) (Dwarf_Macro *, void *),
-			void *arg, ptrdiff_t token, bool accept_0xff)
+ptrdiff_t
+dwarf_getmacros (cudie, callback, arg, token)
+     Dwarf_Die *cudie;
+     int (*callback) (Dwarf_Macro *, void *);
+     void *arg;
+     ptrdiff_t token;
 {
   if (cudie == NULL)
     {
@@ -450,50 +491,6 @@ do_dwarf_getmacros_die (Dwarf_Die *cudie, Dwarf_Off *macoffp,
       return -1;
     }
 
-  if (token > 0 && macoffp != NULL)
-    /* A continuation call from DW_AT_macro_info iteration, meaning
-       *MACOFF contains previously-cached offset.  */
-    return macro_info_getmacros_off (cudie->cu->dbg, *macoffp,
-				     callback, arg, token, cudie);
-
-  /* A fresh start of DW_AT_macro_info iteration, or a continuation
-     thereof without a cache.  */
-  if (token > 0
-      || (token == 0 && dwarf_hasattr (cudie, DW_AT_macro_info)))
-    {
-      Dwarf_Word macoff;
-      if (macoffp == NULL)
-	macoffp = &macoff;
-      if (get_offset_from (cudie, DW_AT_macro_info, macoffp) != 0)
-	return -1;
-      return macro_info_getmacros_off (cudie->cu->dbg, *macoffp,
-				       callback, arg, token, cudie);
-    }
-
-  if (token < 0 && macoffp != NULL)
-    /* A continuation call from DW_AT_GNU_macros iteration.  */
-    return gnu_macros_getmacros_off (cudie->cu->dbg, *macoffp,
-				     callback, arg, token, accept_0xff,
-				     cudie);
-
-  /* Likewise without cache, or iteration start.  */
-  Dwarf_Word macoff;
-  if (macoffp == NULL)
-    macoffp = &macoff;
-  if (get_offset_from (cudie, DW_AT_GNU_macros, macoffp) != 0)
-    return -1;
-  return gnu_macros_getmacros_off (cudie->cu->dbg, *macoffp,
-				   callback, arg, token, accept_0xff,
-				   cudie);
-}
-
-ptrdiff_t
-dwarf_getmacros (die, callback, arg, offset)
-     Dwarf_Die *die;
-     int (*callback) (Dwarf_Macro *, void *);
-     void *arg;
-     ptrdiff_t offset;
-{
   /* This function might be called from a code that expects to see
      DW_MACINFO_* opcodes, not DW_MACRO_{GNU_,}* ones.  It is fine to
      serve most DW_MACRO_{GNU_,}* opcodes to such code, because those
@@ -511,7 +508,33 @@ dwarf_getmacros (die, callback, arg, offset)
      some small probability that the two opcodes would look
      superficially similar enough that a client would be confused and
      misbehave as a result.  For this reason, we refuse to serve
-     through this interface 0xff's originating from .debug_macro.  */
+     through this interface 0xff's originating from .debug_macro
+     unless the TOKEN that we obtained indicates the call originates
+     from a new-style caller.  See above for details on what
+     information is encoded into tokens.  */
+
+  bool accept_0xff;
+  ptrdiff_t offset = offset_from_token (token, &accept_0xff);
+
+  /* DW_AT_macro_info */
+  if (dwarf_hasattr (cudie, DW_AT_macro_info))
+    {
+      Dwarf_Word macoff;
+      if (get_offset_from (cudie, DW_AT_macro_info, &macoff) != 0)
+	return -1;
+      offset = macro_info_getmacros_off (cudie->cu->dbg, macoff,
+					 callback, arg, offset, cudie);
+    }
+  else
+    {
+      /* DW_AT_GNU_macros, DW_AT_macros */
+      Dwarf_Word macoff;
+      if (get_offset_from (cudie, DW_AT_GNU_macros, &macoff) != 0)
+	return -1;
+      offset = gnu_macros_getmacros_off (cudie->cu->dbg, macoff,
+					 callback, arg, offset, accept_0xff,
+					 cudie);
+    }
 
-  return do_dwarf_getmacros_die (die, NULL, callback, arg, offset, false);
+  return token_from_offset (offset, accept_0xff);
 }
diff --git a/libdw/libdw.h b/libdw/libdw.h
index c0baa219..b2b22828 100644
--- a/libdw/libdw.h
+++ b/libdw/libdw.h
@@ -32,6 +32,7 @@
 #include <gelf.h>
 #include <stdbool.h>
 #include <stddef.h>
+#include <stdint.h>
 
 
 #if __GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 3)
@@ -845,22 +846,24 @@ extern int dwarf_entry_breakpoints (Dwarf_Die *die, Dwarf_Addr **bkpts);
 
 
 /* Iterate through the macro unit referenced by CUDIE and call
-   CALLBACK for each macro information entry.  Keeps iterating while
-   CALLBACK returns DWARF_CB_OK.  If the callback returns
-   DWARF_CB_ABORT, it stops iterating and returns a continuation
-   token, which can be used to restart the iteration at the point
-   where it ended.  A TOKEN of 0 starts the iteration.  Returns -1 for
-   errors or 0 if there are no more macro entries.
+   CALLBACK for each macro information entry.  To start the iteration,
+   one would pass DWARF_GETMACROS_START for TOKEN.
+
+   The iteration continues while CALLBACK returns DWARF_CB_OK.  If the
+   callback returns DWARF_CB_ABORT, the iteration stops and a
+   continuation token is returned, which can be used to restart the
+   iteration at the point where it ended.  Returns -1 for errors or 0
+   if there are no more macro entries.
 
    Note that the Dwarf_Macro pointer passed to the callback is only
    valid for the duration of the callback invocation.
 
-   Note that this interface will refuse to serve opcode 0xff from
-   .debug_macro sections.  Such opcode is considered invalid and will
-   cause dwarf_getmacros to return with error.  Note that this should
-   be no limitation as of now, as DW_MACRO_GNU_* domain doesn't
-   allocate 0xff.  It is however a theoretical possibility with future
-   Dwarf standards.  */
+   For backward compatibility, a token of 0 is accepted for starting
+   the iteration as well, but in that case this interface will refuse
+   to serve opcode 0xff from .debug_macro sections.  Such opcode would
+   be considered invalid and would cause dwarf_getmacros to return
+   with error.  */
+#define DWARF_GETMACROS_START PTRDIFF_MIN
 extern ptrdiff_t dwarf_getmacros (Dwarf_Die *cudie,
 				  int (*callback) (Dwarf_Macro *, void *),
 				  void *arg, ptrdiff_t token)
@@ -871,13 +874,16 @@ extern ptrdiff_t dwarf_getmacros (Dwarf_Die *cudie,
    iterates .debug_macro.  This can be used for handling
    DW_MACRO_GNU_transparent_include's or similar opcodes.
 
+   TOKEN value of DWARF_GETMACROS_START can be used to start the
+   iteration.
+
    It is not appropriate to obtain macro unit offset by hand from a CU
    DIE and then request iteration through this interface.  The reason
    for this is that if a dwarf_macro_getsrcfiles is later called,
    there would be no way to figure out what DW_AT_comp_dir was present
    on the CU DIE, and file names referenced in either the macro unit
    itself, or the .debug_line unit that it references, might be wrong.
-   Use dwarf_getmacro.  */
+   Use dwarf_getmacros.  */
 extern ptrdiff_t dwarf_getmacros_off (Dwarf *dbg, Dwarf_Off macoff,
 				      int (*callback) (Dwarf_Macro *, void *),
 				      void *arg, ptrdiff_t token)
@@ -895,9 +901,11 @@ extern int dwarf_macro_getsrcfiles (Dwarf *dbg, Dwarf_Macro *macro,
 /* Return macro opcode.  That's a constant that can be either from
    DW_MACINFO_* domain or DW_MACRO_GNU_* domain.  The two domains have
    compatible values, so it's OK to use either of them for
-   comparisons.  The only differences is 0xff, which currently is
-   never served from .debug_macro, and can thus be safely assumed to
-   mean DW_MACINFO_vendor_ext.  */
+   comparisons.  The only differences is 0xff, which could be either
+   DW_MACINFO_vendor_ext or a vendor-defined DW_MACRO_* constant.  One
+   would need to look if the CU DIE which the iteration was requested
+   for has attribute DW_AT_macro_info, or either of DW_AT_GNU_macros
+   or DW_AT_macros to differentiate the two interpretations.  */
 extern int dwarf_macro_opcode (Dwarf_Macro *macro, unsigned int *opcodep)
      __nonnull_attribute__ (2);
 
diff --git a/tests/ChangeLog b/tests/ChangeLog
index 87a453db..c22ed8a1 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,13 @@
+2014-12-02  Petr Machata  <pmachata@redhat.com>
+
+	* dwarf-getmacros.c (mac): Skip over DW_MACINFO_undef,
+	DW_MACRO_GNU_undef_indirect opcodes.  Add a default branch.
+	(main): Initialize off to DWARF_GETMACROS_START when an extra
+	command line argument is passed.
+	* testfile-macros-0xff.bz2: New test case.
+	* testfile-macros-0xff.s: New file (source for the above).
+	* run-dwarf-getmacros.sh: Add two tests.
+
 2014-11-27  Mark Wielaard  <mjw@redhat.com>
 
 	* vdsosyms.c (main): Call dwfl_linux_proc_attach.
diff --git a/tests/dwarf-getmacros.c b/tests/dwarf-getmacros.c
index f203d5bb..92e093ca 100644
--- a/tests/dwarf-getmacros.c
+++ b/tests/dwarf-getmacros.c
@@ -34,9 +34,9 @@ mac (Dwarf_Macro *macro, void *dbg)
 {
   static int level = 0;
 
-  switch (({ unsigned int opcode;
-	     dwarf_macro_opcode (macro, &opcode);
-	     opcode; }))
+  unsigned int opcode;
+  dwarf_macro_opcode (macro, &opcode);
+  switch (opcode)
     {
     case DW_MACRO_GNU_transparent_include:
       {
@@ -50,7 +50,7 @@ mac (Dwarf_Macro *macro, void *dbg)
 
 	printf ("%*sinclude %#" PRIx64 "\n", level, "", w);
 	++level;
-	include (dbg, w, 0);
+	include (dbg, w, DWARF_GETMACROS_START);
 	--level;
 	printf ("%*s/include\n", level, "");
 	break;
@@ -88,6 +88,19 @@ mac (Dwarf_Macro *macro, void *dbg)
 	printf ("%*s%s\n", level, "", value);
 	break;
       }
+
+    case DW_MACINFO_undef:
+    case DW_MACRO_GNU_undef_indirect:
+      break;
+
+    default:
+      {
+	size_t paramcnt;
+	dwarf_macro_getparamcnt (macro, &paramcnt);
+	printf ("%*sopcode %u with %zd arguments\n",
+		level, "", opcode, paramcnt);
+	break;
+      }
     }
 
   return DWARF_CB_ABORT;
@@ -105,17 +118,19 @@ include (Dwarf *dbg, Dwarf_Off macoff, ptrdiff_t token)
 }
 
 int
-main (int argc __attribute__ ((unused)), char *argv[])
+main (int argc, char *argv[])
 {
+  assert (argc >= 3);
   const char *name = argv[1];
   ptrdiff_t cuoff = strtol (argv[2], NULL, 0);
+  bool new_style = argc > 3;
 
   int fd = open (name, O_RDONLY);
   Dwarf *dbg = dwarf_begin (fd, DWARF_C_READ);
 
   Dwarf_Die cudie_mem, *cudie = dwarf_offdie (dbg, cuoff, &cudie_mem);
 
-  for (ptrdiff_t off = 0;
+  for (ptrdiff_t off = new_style ? DWARF_GETMACROS_START : 0;
        (off = dwarf_getmacros (cudie, mac, dbg, off)); )
     if (off == -1)
       {
diff --git a/tests/run-dwarf-getmacros.sh b/tests/run-dwarf-getmacros.sh
index 8f7c4a80..0a488fa3 100755
--- a/tests/run-dwarf-getmacros.sh
+++ b/tests/run-dwarf-getmacros.sh
@@ -696,4 +696,15 @@ file /home/mark/src/tests/macro.c
 /file
 EOF
 
+testfiles testfile-macros-0xff
+testrun_compare ${abs_builddir}/dwarf-getmacros testfile-macros-0xff 0xb <<\EOF
+invalid opcode
+EOF
+testrun_compare ${abs_builddir}/dwarf-getmacros testfile-macros-0xff 0xb '' <<\EOF
+opcode 255 with 0 arguments
+file /home/petr/proj/elfutils/master/elfutils/x.c
+ FOO 0
+/file
+EOF
+
 exit 0
diff --git a/tests/testfile-macros-0xff.bz2 b/tests/testfile-macros-0xff.bz2
new file mode 100755
index 00000000..a19662a3
--- /dev/null
+++ b/tests/testfile-macros-0xff.bz2
diff --git a/tests/testfile-macros-0xff.s b/tests/testfile-macros-0xff.s
new file mode 100644
index 00000000..7fdd35c2
--- /dev/null
+++ b/tests/testfile-macros-0xff.s
@@ -0,0 +1,153 @@
+	.file	"x.c"
+	.text
+.Ltext0:
+	.globl	main
+	.type	main, @function
+main:
+.LFB0:
+	.file 1 "x.c"
+	.loc 1 3 0
+	.cfi_startproc
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset 6, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register 6
+	.loc 1 3 0
+	movl	$0, %eax
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	main, .-main
+.Letext0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0x52
+	.value	0x4
+	.long	.Ldebug_abbrev0
+	.byte	0x8
+	.uleb128 0x1
+	.long	.LASF244
+	.byte	0x4
+	.string	"x.c"
+	.long	.LASF245
+	.quad	.Ltext0
+	.quad	.Letext0-.Ltext0
+	.long	.Ldebug_line0
+	.long	.Ldebug_macro0
+	.uleb128 0x2
+	.long	.LASF246
+	.byte	0x1
+	.byte	0x3
+	.long	0x4e
+	.quad	.LFB0
+	.quad	.LFE0-.LFB0
+	.uleb128 0x1
+	.byte	0x9c
+	.uleb128 0x3
+	.byte	0x4
+	.byte	0x5
+	.string	"int"
+	.byte	0
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.uleb128 0x1
+	.uleb128 0x11
+	.byte	0x1
+	.uleb128 0x25
+	.uleb128 0xe
+	.uleb128 0x13
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0x8
+	.uleb128 0x1b
+	.uleb128 0xe
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x7
+	.uleb128 0x10
+	.uleb128 0x17
+	.uleb128 0x2119
+	.uleb128 0x17
+	.byte	0
+	.byte	0
+	.uleb128 0x2
+	.uleb128 0x2e
+	.byte	0
+	.uleb128 0x3f
+	.uleb128 0x19
+	.uleb128 0x3
+	.uleb128 0xe
+	.uleb128 0x3a
+	.uleb128 0xb
+	.uleb128 0x3b
+	.uleb128 0xb
+	.uleb128 0x49
+	.uleb128 0x13
+	.uleb128 0x11
+	.uleb128 0x1
+	.uleb128 0x12
+	.uleb128 0x7
+	.uleb128 0x40
+	.uleb128 0x18
+	.uleb128 0x2117
+	.uleb128 0x19
+	.byte	0
+	.byte	0
+	.uleb128 0x3
+	.uleb128 0x24
+	.byte	0
+	.uleb128 0xb
+	.uleb128 0xb
+	.uleb128 0x3e
+	.uleb128 0xb
+	.uleb128 0x3
+	.uleb128 0x8
+	.byte	0
+	.byte	0
+	.byte	0
+	.section	.debug_aranges,"",@progbits
+	.long	0x2c
+	.value	0x2
+	.long	.Ldebug_info0
+	.byte	0x8
+	.byte	0
+	.value	0
+	.value	0
+	.quad	.Ltext0
+	.quad	.Letext0-.Ltext0
+	.quad	0
+	.quad	0
+	.section	.debug_macro,"",@progbits
+.Ldebug_macro0:
+	.value	0x4
+	.byte	0x6
+	.long	.Ldebug_line0
+	.byte   0x1
+	.byte   0xff
+	.uleb128 0
+	.byte   0xff
+	.byte	0x3
+	.uleb128 0
+	.uleb128 0x1
+	.byte	0x5
+	.uleb128 0x1
+	.long	.LASF243
+	.byte	0x4
+	.byte	0
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.section	.debug_str,"MS",@progbits,1
+.LASF245:
+	.string	"/home/petr/proj/elfutils/master/elfutils"
+.LASF244:
+	.string	"GNU C++ 4.9.0 20140422 (Red Hat 4.9.0-1) -mtune=generic -march=x86-64 -g3"
+.LASF243:
+	.string	"FOO 0"
+.LASF246:
+	.string	"main"
+	.ident	"GCC: (GNU) 4.9.0 20140422 (Red Hat 4.9.0-1)"
+	.section	.note.GNU-stack,"",@progbits