[libunwind] Remove unnecessary dependencies on fprintf and stdio.h for increased baremetal friendliness #72040

michael-kenzel · 2023-11-11T21:26:16Z

libunwind already puts logging facilities behind macros so that they can be turned on and off. The two places where this isn't done yet are for debug output tracing during DWARF evaluation and compact unwinding. The code there directly calls fprintf. This becomes a problem when one wants to build libunwind for a baremetal target where fprintf isn't available. This change introduces a set of _LIBUNWIND_TRACE_DWARF_EVAL and _LIBUNWIND_TRACE_COMPACT_UNWIND macros in the style of the already existing macros to remove the direct use of fprintf. It also removes unnecessary includes of stdio.h.

llvmbot · 2023-11-11T21:26:46Z

@llvm/pr-subscribers-libunwind

Author: Michael Kenzel (michael-kenzel)

Changes

libunwind already puts logging facilities behind macros so that they can be turned on and off. The one place where this isn't done yet is for debug output tracing DWARF evaluation. The code there directly calls fprintf. This becomes a problem when one wants to build libunwind for a baremetal target where fprintf isn't available. This change introduces a set of _LIBUNWIND_TRACE_DWARF_EVAL macros in the style of the already existing macros to remove the direct dependency on fprintf.

Full diff: https://github.com/llvm/llvm-project/pull/72040.diff

2 Files Affected:

(modified) libunwind/src/DwarfInstructions.hpp (+55-106)
(modified) libunwind/src/config.h (+15)

diff --git a/libunwind/src/DwarfInstructions.hpp b/libunwind/src/DwarfInstructions.hpp
index 9962c2ffa0ca34f..015489bab1d3ac1 100644
--- a/libunwind/src/DwarfInstructions.hpp
+++ b/libunwind/src/DwarfInstructions.hpp
@@ -381,24 +381,22 @@ typename A::pint_t
 DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
                                             const R &registers,
                                             pint_t initialStackValue) {
-  const bool log = false;
   pint_t p = expression;
   pint_t expressionEnd = expression + 20; // temp, until len read
   pint_t length = (pint_t)addressSpace.getULEB128(p, expressionEnd);
   expressionEnd = p + length;
-  if (log)
-    fprintf(stderr, "evaluateExpression(): length=%" PRIu64 "\n",
-            (uint64_t)length);
+  _LIBUNWIND_TRACE_DWARF_EVAL("evaluateExpression(): length=%" PRIu64 "\n",
+          (uint64_t)length);
   pint_t stack[100];
   pint_t *sp = stack;
   *(++sp) = initialStackValue;
 
   while (p < expressionEnd) {
-    if (log) {
-      for (pint_t *t = sp; t > stack; --t) {
-        fprintf(stderr, "sp[] = 0x%" PRIx64 "\n", (uint64_t)(*t));
-      }
+#if _LIBUNWIND_TRACING_DWARF_EVAL
+    for (pint_t *t = sp; t > stack; --t) {
+      _LIBUNWIND_TRACE_DWARF_EVAL("sp[] = 0x%" PRIx64 "\n", (uint64_t)(*t));
     }
+#endif
     uint8_t opcode = addressSpace.get8(p++);
     sint_t svalue, svalue2;
     pint_t value;
@@ -409,16 +407,14 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       value = addressSpace.getP(p);
       p += sizeof(pint_t);
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "push 0x%" PRIx64 "\n", (uint64_t)value);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push 0x%" PRIx64 "\n", (uint64_t)value);
       break;
 
     case DW_OP_deref:
       // pop stack, dereference, push result
       value = *sp--;
       *(++sp) = addressSpace.getP(value);
-      if (log)
-        fprintf(stderr, "dereference 0x%" PRIx64 "\n", (uint64_t)value);
+      _LIBUNWIND_TRACE_DWARF_EVAL("dereference 0x%" PRIx64 "\n", (uint64_t)value);
       break;
 
     case DW_OP_const1u:
@@ -426,8 +422,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       value = addressSpace.get8(p);
       p += 1;
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "push 0x%" PRIx64 "\n", (uint64_t)value);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push 0x%" PRIx64 "\n", (uint64_t)value);
       break;
 
     case DW_OP_const1s:
@@ -435,8 +430,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       svalue = (int8_t) addressSpace.get8(p);
       p += 1;
       *(++sp) = (pint_t)svalue;
-      if (log)
-        fprintf(stderr, "push 0x%" PRIx64 "\n", (uint64_t)svalue);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push 0x%" PRIx64 "\n", (uint64_t)svalue);
       break;
 
     case DW_OP_const2u:
@@ -444,8 +438,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       value = addressSpace.get16(p);
       p += 2;
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "push 0x%" PRIx64 "\n", (uint64_t)value);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push 0x%" PRIx64 "\n", (uint64_t)value);
       break;
 
     case DW_OP_const2s:
@@ -453,8 +446,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       svalue = (int16_t) addressSpace.get16(p);
       p += 2;
       *(++sp) = (pint_t)svalue;
-      if (log)
-        fprintf(stderr, "push 0x%" PRIx64 "\n", (uint64_t)svalue);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push 0x%" PRIx64 "\n", (uint64_t)svalue);
       break;
 
     case DW_OP_const4u:
@@ -462,8 +454,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       value = addressSpace.get32(p);
       p += 4;
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "push 0x%" PRIx64 "\n", (uint64_t)value);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push 0x%" PRIx64 "\n", (uint64_t)value);
       break;
 
     case DW_OP_const4s:
@@ -471,8 +462,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       svalue = (int32_t)addressSpace.get32(p);
       p += 4;
       *(++sp) = (pint_t)svalue;
-      if (log)
-        fprintf(stderr, "push 0x%" PRIx64 "\n", (uint64_t)svalue);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push 0x%" PRIx64 "\n", (uint64_t)svalue);
       break;
 
     case DW_OP_const8u:
@@ -480,8 +470,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       value = (pint_t)addressSpace.get64(p);
       p += 8;
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "push 0x%" PRIx64 "\n", (uint64_t)value);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push 0x%" PRIx64 "\n", (uint64_t)value);
       break;
 
     case DW_OP_const8s:
@@ -489,47 +478,41 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       value = (pint_t)addressSpace.get64(p);
       p += 8;
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "push 0x%" PRIx64 "\n", (uint64_t)value);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push 0x%" PRIx64 "\n", (uint64_t)value);
       break;
 
     case DW_OP_constu:
       // push immediate ULEB128 value
       value = (pint_t)addressSpace.getULEB128(p, expressionEnd);
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "push 0x%" PRIx64 "\n", (uint64_t)value);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push 0x%" PRIx64 "\n", (uint64_t)value);
       break;
 
     case DW_OP_consts:
       // push immediate SLEB128 value
       svalue = (sint_t)addressSpace.getSLEB128(p, expressionEnd);
       *(++sp) = (pint_t)svalue;
-      if (log)
-        fprintf(stderr, "push 0x%" PRIx64 "\n", (uint64_t)svalue);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push 0x%" PRIx64 "\n", (uint64_t)svalue);
       break;
 
     case DW_OP_dup:
       // push top of stack
       value = *sp;
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "duplicate top of stack\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("duplicate top of stack\n");
       break;
 
     case DW_OP_drop:
       // pop
       --sp;
-      if (log)
-        fprintf(stderr, "pop top of stack\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("pop top of stack\n");
       break;
 
     case DW_OP_over:
       // dup second
       value = sp[-1];
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "duplicate second in stack\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("duplicate second in stack\n");
       break;
 
     case DW_OP_pick:
@@ -538,8 +521,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       p += 1;
       value = sp[-(int)reg];
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "duplicate %d in stack\n", reg);
+      _LIBUNWIND_TRACE_DWARF_EVAL("duplicate %d in stack\n", reg);
       break;
 
     case DW_OP_swap:
@@ -547,8 +529,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       value = sp[0];
       sp[0] = sp[-1];
       sp[-1] = value;
-      if (log)
-        fprintf(stderr, "swap top of stack\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("swap top of stack\n");
       break;
 
     case DW_OP_rot:
@@ -557,133 +538,115 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       sp[0] = sp[-1];
       sp[-1] = sp[-2];
       sp[-2] = value;
-      if (log)
-        fprintf(stderr, "rotate top three of stack\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("rotate top three of stack\n");
       break;
 
     case DW_OP_xderef:
       // pop stack, dereference, push result
       value = *sp--;
       *sp = *((pint_t*)value);
-      if (log)
-        fprintf(stderr, "x-dereference 0x%" PRIx64 "\n", (uint64_t)value);
+      _LIBUNWIND_TRACE_DWARF_EVAL("x-dereference 0x%" PRIx64 "\n", (uint64_t)value);
       break;
 
     case DW_OP_abs:
       svalue = (sint_t)*sp;
       if (svalue < 0)
         *sp = (pint_t)(-svalue);
-      if (log)
-        fprintf(stderr, "abs\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("abs\n");
       break;
 
     case DW_OP_and:
       value = *sp--;
       *sp &= value;
-      if (log)
-        fprintf(stderr, "and\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("and\n");
       break;
 
     case DW_OP_div:
       svalue = (sint_t)(*sp--);
       svalue2 = (sint_t)*sp;
       *sp = (pint_t)(svalue2 / svalue);
-      if (log)
-        fprintf(stderr, "div\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("div\n");
       break;
 
     case DW_OP_minus:
       value = *sp--;
       *sp = *sp - value;
-      if (log)
-        fprintf(stderr, "minus\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("minus\n");
       break;
 
     case DW_OP_mod:
       svalue = (sint_t)(*sp--);
       svalue2 = (sint_t)*sp;
       *sp = (pint_t)(svalue2 % svalue);
-      if (log)
-        fprintf(stderr, "module\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("module\n");
       break;
 
     case DW_OP_mul:
       svalue = (sint_t)(*sp--);
       svalue2 = (sint_t)*sp;
       *sp = (pint_t)(svalue2 * svalue);
-      if (log)
-        fprintf(stderr, "mul\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("mul\n");
       break;
 
     case DW_OP_neg:
       *sp = 0 - *sp;
-      if (log)
-        fprintf(stderr, "neg\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("neg\n");
       break;
 
     case DW_OP_not:
       svalue = (sint_t)(*sp);
       *sp = (pint_t)(~svalue);
-      if (log)
-        fprintf(stderr, "not\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("not\n");
       break;
 
     case DW_OP_or:
       value = *sp--;
       *sp |= value;
-      if (log)
-        fprintf(stderr, "or\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("or\n");
       break;
 
     case DW_OP_plus:
       value = *sp--;
       *sp += value;
-      if (log)
-        fprintf(stderr, "plus\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("plus\n");
       break;
 
     case DW_OP_plus_uconst:
       // pop stack, add uelb128 constant, push result
       *sp += static_cast<pint_t>(addressSpace.getULEB128(p, expressionEnd));
-      if (log)
-        fprintf(stderr, "add constant\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("add constant\n");
       break;
 
     case DW_OP_shl:
       value = *sp--;
       *sp = *sp << value;
-      if (log)
-        fprintf(stderr, "shift left\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("shift left\n");
       break;
 
     case DW_OP_shr:
       value = *sp--;
       *sp = *sp >> value;
-      if (log)
-        fprintf(stderr, "shift left\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("shift left\n");
       break;
 
     case DW_OP_shra:
       value = *sp--;
       svalue = (sint_t)*sp;
       *sp = (pint_t)(svalue >> value);
-      if (log)
-        fprintf(stderr, "shift left arithmetic\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("shift left arithmetic\n");
       break;
 
     case DW_OP_xor:
       value = *sp--;
       *sp ^= value;
-      if (log)
-        fprintf(stderr, "xor\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("xor\n");
       break;
 
     case DW_OP_skip:
       svalue = (int16_t) addressSpace.get16(p);
       p += 2;
       p = (pint_t)((sint_t)p + svalue);
-      if (log)
-        fprintf(stderr, "skip %" PRIu64 "\n", (uint64_t)svalue);
+      _LIBUNWIND_TRACE_DWARF_EVAL("skip %" PRIu64 "\n", (uint64_t)svalue);
       break;
 
     case DW_OP_bra:
@@ -691,50 +654,43 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       p += 2;
       if (*sp--)
         p = (pint_t)((sint_t)p + svalue);
-      if (log)
-        fprintf(stderr, "bra %" PRIu64 "\n", (uint64_t)svalue);
+      _LIBUNWIND_TRACE_DWARF_EVAL("bra %" PRIu64 "\n", (uint64_t)svalue);
       break;
 
     case DW_OP_eq:
       value = *sp--;
       *sp = (*sp == value);
-      if (log)
-        fprintf(stderr, "eq\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("eq\n");
       break;
 
     case DW_OP_ge:
       value = *sp--;
       *sp = (*sp >= value);
-      if (log)
-        fprintf(stderr, "ge\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("ge\n");
       break;
 
     case DW_OP_gt:
       value = *sp--;
       *sp = (*sp > value);
-      if (log)
-        fprintf(stderr, "gt\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("gt\n");
       break;
 
     case DW_OP_le:
       value = *sp--;
       *sp = (*sp <= value);
-      if (log)
-        fprintf(stderr, "le\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("le\n");
       break;
 
     case DW_OP_lt:
       value = *sp--;
       *sp = (*sp < value);
-      if (log)
-        fprintf(stderr, "lt\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("lt\n");
       break;
 
     case DW_OP_ne:
       value = *sp--;
       *sp = (*sp != value);
-      if (log)
-        fprintf(stderr, "ne\n");
+      _LIBUNWIND_TRACE_DWARF_EVAL0("ne\n");
       break;
 
     case DW_OP_lit0:
@@ -771,8 +727,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
     case DW_OP_lit31:
       value = static_cast<pint_t>(opcode - DW_OP_lit0);
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "push literal 0x%" PRIx64 "\n", (uint64_t)value);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push literal 0x%" PRIx64 "\n", (uint64_t)value);
       break;
 
     case DW_OP_reg0:
@@ -809,15 +764,13 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
     case DW_OP_reg31:
       reg = static_cast<uint32_t>(opcode - DW_OP_reg0);
       *(++sp) = registers.getRegister((int)reg);
-      if (log)
-        fprintf(stderr, "push reg %d\n", reg);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push reg %d\n", reg);
       break;
 
     case DW_OP_regx:
       reg = static_cast<uint32_t>(addressSpace.getULEB128(p, expressionEnd));
       *(++sp) = registers.getRegister((int)reg);
-      if (log)
-        fprintf(stderr, "push reg %d + 0x%" PRIx64 "\n", reg, (uint64_t)svalue);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push reg %d + 0x%" PRIx64 "\n", reg, (uint64_t)svalue);
       break;
 
     case DW_OP_breg0:
@@ -856,8 +809,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       svalue = (sint_t)addressSpace.getSLEB128(p, expressionEnd);
       svalue += static_cast<sint_t>(registers.getRegister((int)reg));
       *(++sp) = (pint_t)(svalue);
-      if (log)
-        fprintf(stderr, "push reg %d + 0x%" PRIx64 "\n", reg, (uint64_t)svalue);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push reg %d + 0x%" PRIx64 "\n", reg, (uint64_t)svalue);
       break;
 
     case DW_OP_bregx:
@@ -865,8 +817,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
       svalue = (sint_t)addressSpace.getSLEB128(p, expressionEnd);
       svalue += static_cast<sint_t>(registers.getRegister((int)reg));
       *(++sp) = (pint_t)(svalue);
-      if (log)
-        fprintf(stderr, "push reg %d + 0x%" PRIx64 "\n", reg, (uint64_t)svalue);
+      _LIBUNWIND_TRACE_DWARF_EVAL("push reg %d + 0x%" PRIx64 "\n", reg, (uint64_t)svalue);
       break;
 
     case DW_OP_fbreg:
@@ -897,8 +848,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
         _LIBUNWIND_ABORT("DW_OP_deref_size with bad size");
       }
       *(++sp) = value;
-      if (log)
-        fprintf(stderr, "sized dereference 0x%" PRIx64 "\n", (uint64_t)value);
+      _LIBUNWIND_TRACE_DWARF_EVAL("sized dereference 0x%" PRIx64 "\n", (uint64_t)value);
       break;
 
     case DW_OP_xderef_size:
@@ -912,8 +862,7 @@ DwarfInstructions<A, R>::evaluateExpression(pint_t expression, A &addressSpace,
     }
 
   }
-  if (log)
-    fprintf(stderr, "expression evaluates to 0x%" PRIx64 "\n", (uint64_t)*sp);
+  _LIBUNWIND_TRACE_DWARF_EVAL("expression evaluates to 0x%" PRIx64 "\n", (uint64_t)*sp);
   return *sp;
 }
 
diff --git a/libunwind/src/config.h b/libunwind/src/config.h
index deb5a4d4d73d467..c31d4113401cd03 100644
--- a/libunwind/src/config.h
+++ b/libunwind/src/config.h
@@ -223,6 +223,21 @@
     } while (0)
 #endif
 
+#define _LIBUNWIND_TRACING_DWARF_EVAL (0)
+#if !_LIBUNWIND_TRACING_DWARF_EVAL
+#define _LIBUNWIND_TRACE_DWARF_EVAL0(msg)
+#define _LIBUNWIND_TRACE_DWARF_EVAL(msg, ...)
+#else
+#define _LIBUNWIND_TRACE_DWARF_EVAL0(msg)                                      \
+    do {                                                                       \
+      fprintf(stderr, msg);                                                    \
+    } while(0)
+#define _LIBUNWIND_TRACE_DWARF_EVAL(msg, ...)                                  \
+    do {                                                                       \
+      fprintf(stderr, msg, __VA_ARGS__);                                       \
+    } while(0)
+#endif
+
 #ifdef __cplusplus
 // Used to fit UnwindCursor and Registers_xxx types against unw_context_t /
 // unw_cursor_t sized memory blocks.

arichardson

I'm happy with this as is but I'd wait for at least one other positive review before merging.

libunwind/src/config.h

github-actions · 2023-11-12T00:53:52Z

✅ With the latest revision this PR passed the C/C++ code formatter.

libunwind/src/DwarfInstructions.hpp

ldionne

This looks like an improvement to me. I have a comment/question about the stdio.h include which is now dangling, but apart from that I'm happy with the patch.

michael-kenzel · 2023-11-19T05:39:11Z

As discussed, I've removed unnecessary includes of <stdio.h> as well as a couple other unnecessary includes. I've put the include of <stdio.h> in config.h under an #ifdef so it only gets included when logging is potentially active. I've also moved the include of <assert.h> from config.h into only the files that actually need it. In two places, I replaced the include <stdio.h> with an include of the header that was actually needed (<stdint.h> or <string.h>).

AddressSpace.hpp was using snprintf to copy strings into a buffer. I replaced those uses with strncpy to remove the need for <stdio.h>.

Finally, I've added _LIBUNWIND_TRACE_COMPACT_UNWIND to analogously deal with the debug logging in UnwindCursor.hpp.

I've updated the title of the commit and PR to reflect the now extended scope of these changes.

Unfortunately, I can't really exercise all these code paths here as these changes now span code specific to multiple different platforms. I'll trust that if all the tests pass, that means it works…

libunwind/src/AddressSpace.hpp

michael-kenzel · 2023-11-20T22:03:07Z

AddressSpace.hpp was using snprintf to copy strings into a buffer. I replaced those uses with strncpy to remove the need for <stdio.h>.

As discussed with @arichardson, I've replaced strncpy with strlen + memcpy.

arichardson

Thanks! LGTM once CI is happy.

ldionne · 2023-11-21T15:12:44Z

The CI issues are flakes (we just made major changes to our infrastructure and we're still adjusting). I think this is good to go.

TiborGY · 2025-02-18T06:46:42Z

@michael-kenzel Was there a reason why this never got merged?

arichardson

This looks like a nice improvement. Some minor questions inline

arichardson · 2025-02-18T20:04:40Z

libunwind/src/DwarfInstructions.hpp

-      for (pint_t *t = sp; t > stack; --t) {
-        fprintf(stderr, "sp[] = 0x%" PRIx64 "\n", (uint64_t)(*t));
-      }
+#if _LIBUNWIND_TRACING_DWARF_EVAL


Is this needed? Can't the compiler elide the loop if _LIBUNWIND_TRACE_DWARF_EVAL is a no-op?

I would expect the compiler to optimize the loop away in an optimized build. If I remember correctly, and it's been a while, then the reason I did it this way was just to avoid generating a pointless loop also in an unoptimized build.

arichardson · 2025-02-18T20:06:03Z

libunwind/src/config.h

+#define _LIBUNWIND_TRACE_COMPACT_UNWIND0(msg)                                  \
+  do {                                                                         \
+    fprintf(stderr, msg);                                                      \
+  } while (0)
+#define _LIBUNWIND_TRACE_COMPACT_UNWIND(msg, ...)                              \
+  do {                                                                         \
+    fprintf(stderr, msg, __VA_ARGS__);                                         \
+  } while (0)


Suggested change

#define _LIBUNWIND_TRACE_COMPACT_UNWIND0(msg) \

do { \

fprintf(stderr, msg); \

} while (0)

#define _LIBUNWIND_TRACE_COMPACT_UNWIND(msg, ...) \

do { \

fprintf(stderr, msg, __VA_ARGS__); \

} while (0)

#define _LIBUNWIND_TRACE_COMPACT_UNWIND(...) \

do { \

fprintf(stderr, __VA_ARGS__); \

} while (0)

Does this work?

I think it should; the reason I did it the way I did was consistency with the existing _LIBUNWIND_LOG/_LIBUNWIND_LOG0 macros. I guess best would be to just rewrite those in your suggested way as well. But maybe this should be done in a separate PR to keep this one focused on just making fprintf optional?

Sure fine by me :)

arichardson · 2025-02-18T20:06:15Z

libunwind/src/config.h

+#define _LIBUNWIND_TRACE_DWARF_EVAL0(msg)
+#define _LIBUNWIND_TRACE_DWARF_EVAL(msg, ...)
+#else
+#define _LIBUNWIND_TRACE_DWARF_EVAL0(msg)                                      \


Same as above?

same as above, yes

michael-kenzel · 2025-02-18T20:51:11Z

@michael-kenzel Was there a reason why this never got merged?

I don't think there was, I guess this just fell through the cracks…

I'll rebase this PR onto the current main once I find some time to do so.

…baremetal friendliness

michael-kenzel · 2025-03-07T14:55:21Z

@arichardson as promised, I have rebased the changes. I'm not sure about the test failures, it would seem to me that they are unlikely to be caused by anything I did?

arichardson · 2025-03-07T18:35:46Z

@arichardson as promised, I have rebased the changes. I'm not sure about the test failures, it would seem to me that they are unlikely to be caused by anything I did?

Yes it looks like an issue with the android CI setup, nothing related to this patch I believe.

michael-kenzel requested a review from a team as a code owner November 11, 2023 21:26

llvmbot added the libunwind label Nov 11, 2023

arichardson approved these changes Nov 11, 2023

View reviewed changes

libunwind/src/config.h Outdated Show resolved Hide resolved

michael-kenzel force-pushed the libunwind-remove-fprintf branch 3 times, most recently from 0074210 to 7e285a5 Compare November 12, 2023 03:05

michael-kenzel changed the title ~~[libunwind] introduce _LIBUNWIND_TRACE_DWARF_EVAL for increased baremetal friendliness~~ [libunwind] Introduce _LIBUNWIND_TRACE_DWARF_EVAL for increased baremetal friendliness Nov 12, 2023

ldionne reviewed Nov 16, 2023

View reviewed changes

libunwind/src/DwarfInstructions.hpp Outdated Show resolved Hide resolved

ldionne approved these changes Nov 16, 2023

View reviewed changes

michael-kenzel marked this pull request as draft November 17, 2023 01:22

michael-kenzel force-pushed the libunwind-remove-fprintf branch 5 times, most recently from f98bbd2 to 0bedd8b Compare November 19, 2023 04:49

michael-kenzel changed the title ~~[libunwind] Introduce _LIBUNWIND_TRACE_DWARF_EVAL for increased baremetal friendliness~~ [libunwind] Remove unnecessary dependencies on fprintf and stdio.h for increased baremetal friendliness Nov 19, 2023

michael-kenzel force-pushed the libunwind-remove-fprintf branch 2 times, most recently from a72cef2 to f8b562d Compare November 19, 2023 05:20

michael-kenzel closed this Nov 19, 2023

michael-kenzel reopened this Nov 19, 2023

michael-kenzel marked this pull request as ready for review November 19, 2023 05:39

arichardson reviewed Nov 20, 2023

View reviewed changes

libunwind/src/AddressSpace.hpp Outdated Show resolved Hide resolved

arichardson reviewed Nov 20, 2023

View reviewed changes

libunwind/src/AddressSpace.hpp Outdated Show resolved Hide resolved

michael-kenzel force-pushed the libunwind-remove-fprintf branch from f8b562d to fcdccb3 Compare November 20, 2023 21:56

arichardson approved these changes Nov 20, 2023

View reviewed changes

arichardson reviewed Feb 18, 2025

View reviewed changes

michael-kenzel force-pushed the libunwind-remove-fprintf branch from fcdccb3 to 8c59516 Compare March 7, 2025 10:56

[libunwind] Remove unnecessary dependencies on stdio.h for increased …

70ef857

…baremetal friendliness

michael-kenzel force-pushed the libunwind-remove-fprintf branch from 8c59516 to 70ef857 Compare March 7, 2025 11:11

arichardson approved these changes Mar 7, 2025

View reviewed changes

[libunwind] Remove unnecessary dependencies on fprintf and stdio.h for increased baremetal friendliness #72040

Are you sure you want to change the base?

[libunwind] Remove unnecessary dependencies on fprintf and stdio.h for increased baremetal friendliness #72040

Uh oh!

Conversation

michael-kenzel commented Nov 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 11, 2023

Uh oh!

arichardson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ldionne left a comment

Choose a reason for hiding this comment

Uh oh!

michael-kenzel commented Nov 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michael-kenzel commented Nov 20, 2023

Uh oh!

arichardson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldionne commented Nov 21, 2023

Uh oh!

TiborGY commented Feb 18, 2025

Uh oh!

arichardson left a comment

Choose a reason for hiding this comment

Uh oh!

arichardson Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

michael-kenzel Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

arichardson Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

michael-kenzel Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arichardson Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

arichardson Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

michael-kenzel Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-kenzel commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-kenzel commented Mar 7, 2025

Uh oh!

arichardson commented Mar 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

michael-kenzel commented Nov 11, 2023 •

edited

Loading

github-actions bot commented Nov 12, 2023 •

edited

Loading

michael-kenzel commented Nov 19, 2023 •

edited

Loading

arichardson left a comment •

edited

Loading

michael-kenzel Feb 18, 2025 •

edited

Loading

michael-kenzel Feb 18, 2025 •

edited

Loading

michael-kenzel commented Feb 18, 2025 •

edited

Loading