This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] readelf.c: Use real output width [Re: [patch] Implement reglocs for s390/s390x]


On Thu, 11 Oct 2012 00:04:01 +0200, Roland McGrath wrote:
> > Yes, bad will happen:
> > 
> >     info.si_signo: 6, info.si_code: 0, info.si_errno: 0, cursig: 6
> > ->
> >     info.si_signo: 6, info.si_code: 0
> >     info.si_errno: 0, cursig: 6
> > 
> > etc., everything gets too narrow.
> > 
> > Because max_width is very exaggerated there while the values here like "0" or
> > "6" are in reality much shorter.
> 
> Ah, right.  We don't wrap based on the actual output, but based on an
> expectation of the potential size of each field.  I think that's right
> so that each field appears on the same line regardless of the values.
> 
> I don't seem to be understanding your code.  The comment still says that
> it's avoiding having the wrapping depend on the particular values.  But I
> can't see in the code what does that.  print_core_item itself clearly now
> depends on the size of the formatted string for each value.  The comment
> says, "the FORMAT strings typically already contain their maximum width."
> But I don't see how that's so.  What am I missing?

The "debug dump" I provided:
    ebx:          21687<11:%11d>  ecx:          21689<11:%11d>  edx:              6<11:%11d>
    esi:              0<11:%11d>  edi:     -143585292<11:%11d>  ebp:     0xf7562308<11: 0x%.8x>
    eax:              0<11:%11d>  eip:     0xf774d430<11: 0x%.8x>  eflags:  0x00000296<11: 0x%.8x>

shows both FORMAT_MAX and FORMAT used for each field:
	printf ("<%zu:%s>", format_max, format);

One can see FORMAT "%11d" (or "0x %.8x") is enough to keep the output aligned,
without the need for additional FORMAT_MAX.

In the case of si_signo/si_code:
    info.si_signo: 6<11:%d>, info.si_code: 0<11:%d>, info.si_errno: 0<11:%d>, cursig: 6<6:%d>

There is already just "%d" which also seems to match what we want.


> I prefer the old output: sigpend and sighold on separate lines,
> sid on the same like with *id, pgrp.

I do not see any change now on the files I tried.


> I wonder what would happen if we just set the format-max values for the
> decimal formats smaller, so they're not really the maximum width but
> something more like the "common" width.

Then it will be sometimes unaligned.

I really do not mind much, we got far away from the goal of upstreaming an
unwinder.


jankratochvil/readelf-width


Thanks,
Jan


commit 0790870c9a564ee9797f8e8668085cb38ebded95
Author: Jan Kratochvil <jan.kratochvil@redhat.com>
Date:   Wed Oct 10 22:27:58 2012 +0200

    src/
    2012-10-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
    
    	* readelf.c (ITEM_WRAP_COLUMN, REGISTER_WRAP_COLUMN): Merge to ...
    	(WRAP_COLUMN): ... here.
    	(print_core_item): Remove parameter format_max.  Update function
    	comment.  Replace FORMAT_MAX by the real output width.
    	(handle_core_item): Remove the FORMAT_MAX values in TYPES, DO_TYPE,
    	calls of print_core_item, remove variable maxfmt, change
    	ITEM_WRAP_COLUMN to WRAP_COLUMN.
    	(handle_core_register): Remove the FORMAT_MAX values in TYPES, BITS,
    	calls of print_core_item, change REGISTER_WRAP_COLUMN to WRAP_COLUMN.
    
    backends/
    2012-10-11  Jan Kratochvil  <jan.kratochvil@redhat.com>
    
    	* linux-core-note.c (prstatus_items): Rename groups of sigpend and
    	sighold to signal2 and signal3.
    
    Signed-off-by: Jan Kratochvil <jan.kratochvil@redhat.com>

diff --git a/backends/linux-core-note.c b/backends/linux-core-note.c
index 1592694..b09154f 100644
--- a/backends/linux-core-note.c
+++ b/backends/linux-core-note.c
@@ -123,8 +123,10 @@ static const Ebl_Core_Item prstatus_items[] =
     FIELD (signal, INT, info.si_code, 'd'),
     FIELD (signal, INT, info.si_errno, 'd'),
     FIELD (signal, SHORT, cursig, 'd'),
-    FIELD (signal, ULONG, sigpend, 'B'),
-    FIELD (signal, ULONG, sighold, 'B'),
+
+    /* Use different group name for a newline delimiter.  */
+    FIELD (signal2, ULONG, sigpend, 'B'),
+    FIELD (signal3, ULONG, sighold, 'B'),
     FIELD (identity, PID_T, pid, 'd', .thread_identifier = true),
     FIELD (identity, PID_T, ppid, 'd'),
     FIELD (identity, PID_T, pgrp, 'd'),
diff --git a/src/readelf.c b/src/readelf.c
index 5d167eb..2eb85f4 100644
--- a/src/readelf.c
+++ b/src/readelf.c
@@ -7337,24 +7337,31 @@ print_debug (Dwfl_Module *dwflmod, Ebl *ebl, GElf_Ehdr *ehdr)
 
 
 #define ITEM_INDENT		4
-#define ITEM_WRAP_COLUMN	150
-#define REGISTER_WRAP_COLUMN	75
+#define WRAP_COLUMN		75
+
+/* Print "NAME: FORMAT", wrapping when output text would make the line exceed
+   WRAP_COLUMN.  We do not want the line breaks to depend on the particular
+   values, this is why the FORMAT strings typically already contain their
+   maximum width.  */
 
-/* Print "NAME: FORMAT", wrapping when FORMAT_MAX chars of FORMAT would
-   make the line exceed ITEM_WRAP_COLUMN.  Unpadded numbers look better
-   for the core items.  But we do not want the line breaks to depend on
-   the particular values.  */
 static unsigned int
-__attribute__ ((format (printf, 7, 8)))
+__attribute__ ((format (printf, 6, 7)))
 print_core_item (unsigned int colno, char sep, unsigned int wrap,
-		 size_t name_width, const char *name,
-		 size_t format_max, const char *format, ...)
+		 size_t name_width, const char *name, const char *format, ...)
 {
   size_t len = strlen (name);
   if (name_width < len)
     name_width = len;
 
-  size_t n = name_width + sizeof ": " - 1 + format_max;
+  char *out;
+  va_list ap;
+  va_start (ap, format);
+  int out_len = vasprintf (&out, format, ap);
+  va_end (ap);
+  if (out_len == -1)
+    error (EXIT_FAILURE, 0, _("memory exhausted"));
+
+  size_t n = name_width + sizeof ": " - 1 + out_len;
 
   if (colno == 0)
     {
@@ -7372,12 +7379,9 @@ print_core_item (unsigned int colno, char sep, unsigned int wrap,
       colno = ITEM_INDENT + n;
     }
 
-  printf ("%s: %*s", name, (int) (name_width - len), "");
+  printf ("%s: %*s%s", name, (int) (name_width - len), "", out);
 
-  va_list ap;
-  va_start (ap, format);
-  vprintf (format, ap);
-  va_end (ap);
+  free (out);
 
   return colno;
 }
@@ -7420,14 +7424,14 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
   uint_fast16_t count = item->count ?: 1;
 
 #define TYPES								      \
-  DO_TYPE (BYTE, Byte, "0x%.2" PRIx8, "%" PRId8, 4);			      \
-  DO_TYPE (HALF, Half, "0x%.4" PRIx16, "%" PRId16, 6);			      \
-  DO_TYPE (WORD, Word, "0x%.8" PRIx32, "%" PRId32, 11);			      \
-  DO_TYPE (SWORD, Sword, "%" PRId32, "%" PRId32, 11);			      \
-  DO_TYPE (XWORD, Xword, "0x%.16" PRIx64, "%" PRId64, 20);		      \
-  DO_TYPE (SXWORD, Sxword, "%" PRId64, "%" PRId64, 20)
-
-#define DO_TYPE(NAME, Name, hex, dec, max) GElf_##Name Name[count]
+  DO_TYPE (BYTE, Byte, "0x%.2" PRIx8, "%" PRId8);			      \
+  DO_TYPE (HALF, Half, "0x%.4" PRIx16, "%" PRId16);			      \
+  DO_TYPE (WORD, Word, "0x%.8" PRIx32, "%" PRId32);			      \
+  DO_TYPE (SWORD, Sword, "%" PRId32, "%" PRId32);			      \
+  DO_TYPE (XWORD, Xword, "0x%.16" PRIx64, "%" PRId64);			      \
+  DO_TYPE (SXWORD, Sxword, "%" PRId64, "%" PRId64)
+
+#define DO_TYPE(NAME, Name, hex, dec) GElf_##Name Name[count]
   union { TYPES; } value;
 #undef DO_TYPE
 
@@ -7459,10 +7463,10 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
       assert (count == 1);
       switch (type)
 	{
-#define DO_TYPE(NAME, Name, hex, dec, max)				      \
+#define DO_TYPE(NAME, Name, hex, dec)					      \
 	  case ELF_T_##NAME:						      \
-	    colno = print_core_item (colno, ',', ITEM_WRAP_COLUMN,	      \
-				     0, item->name, max, dec, value.Name[0]); \
+	    colno = print_core_item (colno, ',', WRAP_COLUMN,		      \
+				     0, item->name, dec, value.Name[0]); \
 	    break
 	  TYPES;
 #undef DO_TYPE
@@ -7475,10 +7479,10 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
       assert (count == 1);
       switch (type)
 	{
-#define DO_TYPE(NAME, Name, hex, dec, max)				      \
+#define DO_TYPE(NAME, Name, hex, dec)					      \
 	  case ELF_T_##NAME:						      \
-	    colno = print_core_item (colno, ',', ITEM_WRAP_COLUMN,	      \
-				     0, item->name, max, hex, value.Name[0]); \
+	    colno = print_core_item (colno, ',', WRAP_COLUMN,		      \
+				     0, item->name, hex, value.Name[0]);      \
 	    break
 	  TYPES;
 #undef DO_TYPE
@@ -7546,8 +7550,7 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
 	if (lastbit > 0 && lastbit + 1 != nbits)
 	  p += sprintf (p, "-%u", nbits - bias);
 
-	colno = print_core_item (colno, ',', ITEM_WRAP_COLUMN, 0, item->name,
-				 4 + nbits * 4,
+	colno = print_core_item (colno, ',', WRAP_COLUMN, 0, item->name,
 				 negate ? "~<%s>" : "<%s>", printed);
       }
       break;
@@ -7557,14 +7560,12 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
       assert (count == 2);
       Dwarf_Word sec;
       Dwarf_Word usec;
-      size_t maxfmt = 7;
       switch (type)
 	{
-#define DO_TYPE(NAME, Name, hex, dec, max)				      \
+#define DO_TYPE(NAME, Name, hex, dec)					      \
 	  case ELF_T_##NAME:						      \
 	    sec = value.Name[0];					      \
 	    usec = value.Name[1];					      \
-	    maxfmt += max;						      \
 	    break
 	  TYPES;
 #undef DO_TYPE
@@ -7587,19 +7588,19 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
 	  else
 	    usec &= UINT32_MAX;
 	}
-      colno = print_core_item (colno, ',', ITEM_WRAP_COLUMN, 0, item->name,
-			       maxfmt, "%" PRIu64 ".%.6" PRIu64, sec, usec);
+      colno = print_core_item (colno, ',', WRAP_COLUMN, 0, item->name,
+			       "%" PRIu64 ".%.6" PRIu64, sec, usec);
       break;
 
     case 'c':
       assert (count == 1);
-      colno = print_core_item (colno, ',', ITEM_WRAP_COLUMN, 0, item->name,
-			       1, "%c", value.Byte[0]);
+      colno = print_core_item (colno, ',', WRAP_COLUMN, 0, item->name,
+			       "%c", value.Byte[0]);
       break;
 
     case 's':
-      colno = print_core_item (colno, ',', ITEM_WRAP_COLUMN, 0, item->name,
-			       count, "%.*s", (int) count, value.Byte);
+      colno = print_core_item (colno, ',', WRAP_COLUMN, 0, item->name,
+			       "%.*s", (int) count, value.Byte);
       break;
 
     case '\n':
@@ -7626,7 +7627,7 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
 	  s = eol + 1;
 	}
 
-      colno = ITEM_WRAP_COLUMN;
+      colno = WRAP_COLUMN;
       break;
 
     default:
@@ -7722,7 +7723,7 @@ handle_core_items (Elf *core, const void *desc, size_t descsz,
 	    colno = handle_core_item (core, *item, desc, colno, NULL);
 
 	  /* Force a line break at the end of the group.  */
-	  colno = ITEM_WRAP_COLUMN;
+	  colno = WRAP_COLUMN;
 	}
 
       if (descsz == 0)
@@ -7790,12 +7791,12 @@ handle_core_register (Ebl *ebl, Elf *core, int maxregname,
       register_info (ebl, reg, regloc, name, &bits, &type);
 
 #define TYPES								      \
-      BITS (8, BYTE, "%4" PRId8, "0x%.2" PRIx8, 4);			      \
-      BITS (16, HALF, "%6" PRId16, "0x%.4" PRIx16, 6);			      \
-      BITS (32, WORD, "%11" PRId32, " 0x%.8" PRIx32, 11);		      \
-      BITS (64, XWORD, "%20" PRId64, "  0x%.16" PRIx64, 20)
+      BITS (8, BYTE, "%4" PRId8, "0x%.2" PRIx8);			      \
+      BITS (16, HALF, "%6" PRId16, "0x%.4" PRIx16);			      \
+      BITS (32, WORD, "%11" PRId32, " 0x%.8" PRIx32);			      \
+      BITS (64, XWORD, "%20" PRId64, "  0x%.16" PRIx64)
 
-#define BITS(bits, xtype, sfmt, ufmt, max)				\
+#define BITS(bits, xtype, sfmt, ufmt)				\
       uint##bits##_t b##bits; int##bits##_t b##bits##s
       union { TYPES; uint64_t b128[2]; } value;
 #undef	BITS
@@ -7807,17 +7808,17 @@ handle_core_register (Ebl *ebl, Elf *core, int maxregname,
 	case DW_ATE_address:
 	  switch (bits)
 	    {
-#define BITS(bits, xtype, sfmt, ufmt, max)				      \
+#define BITS(bits, xtype, sfmt, ufmt)					      \
 	    case bits:							      \
 	      desc = convert (core, ELF_T_##xtype, 1, &value, desc, 0);	      \
 	      if (type == DW_ATE_signed)				      \
-		colno = print_core_item (colno, ' ', REGISTER_WRAP_COLUMN,    \
+		colno = print_core_item (colno, ' ', WRAP_COLUMN,	      \
 					 maxregname, name,		      \
-					 max, sfmt, value.b##bits##s);	      \
+					 sfmt, value.b##bits##s);	      \
 	      else							      \
-		colno = print_core_item (colno, ' ', REGISTER_WRAP_COLUMN,    \
+		colno = print_core_item (colno, ' ', WRAP_COLUMN,	      \
 					 maxregname, name,		      \
-					 max, ufmt, value.b##bits);	      \
+					 ufmt, value.b##bits);		      \
 	      break
 
 	    TYPES;
@@ -7826,9 +7827,9 @@ handle_core_register (Ebl *ebl, Elf *core, int maxregname,
 	      assert (type == DW_ATE_unsigned);
 	      desc = convert (core, ELF_T_XWORD, 2, &value, desc, 0);
 	      int be = elf_getident (core, NULL)[EI_DATA] == ELFDATA2MSB;
-	      colno = print_core_item (colno, ' ', REGISTER_WRAP_COLUMN,
+	      colno = print_core_item (colno, ' ', WRAP_COLUMN,
 				       maxregname, name,
-				       34, "0x%.16" PRIx64 "%.16" PRIx64,
+				       "0x%.16" PRIx64 "%.16" PRIx64,
 				       value.b128[!be], value.b128[be]);
 	      break;
 
@@ -7857,9 +7858,8 @@ handle_core_register (Ebl *ebl, Elf *core, int maxregname,
 	      *h++ = "0123456789abcdef"[bytes[idx] >> 4];
 	      *h++ = "0123456789abcdef"[bytes[idx] & 0xf];
 	    }
-	  colno = print_core_item (colno, ' ', REGISTER_WRAP_COLUMN,
-				   maxregname, name,
-				   2 + sizeof hex - 1, "0x%s", hex);
+	  colno = print_core_item (colno, ' ', WRAP_COLUMN,
+				   maxregname, name, "0x%s", hex);
 	  break;
 	}
       desc += regloc->pad;
@@ -7998,7 +7998,7 @@ handle_core_registers (Ebl *ebl, Elf *core, const void *desc,
 				      reg->regloc, desc, colno);
 
       /* Force a line break at the end of the group.  */
-      colno = REGISTER_WRAP_COLUMN;
+      colno = WRAP_COLUMN;
     }
 
   return colno;

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]