[libunwind] Remove unnecessary strcpy dependency

[michael-kenzel]

libunwind uses a minimal set of necessary standard library functions, basically just memset and memcpy. There is a single use of strcpy to copy the bytes "CLNGUNW" into a uint64_t object. This is both an arguably odd use of the strcpy function as well as it unnecessarily widens the set of library functions that must be available to build libunwind, which can be an obstacle in baremetal scenarios. This change simply replaces this one strcpy with the more fundamental memcpy.

[llvmbot]

@llvm/pr-subscribers-libunwind

Author: Michael Kenzel (michael-kenzel)

Changes

libunwind uses a minimum set of necessary standard library functions, basically just memset and memcpy. There is a single use of strcpy to copy the bytes "CLNGUNW" into a uint64_t object. This is both an arguably odd use of the strcpy function as well as it unnecessarily widens the set of library functions that must be available to build libunwind, which can be an obstacle in baremetal scenarios. This change simply replaces this one strcpy with the more fundamental memcpy.

---

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

1 Files Affected:

• (modified) libunwind/src/UnwindLevel1-gcc-ext.c (+1-1)

diff --git a/libunwind/src/UnwindLevel1-gcc-ext.c b/libunwind/src/UnwindLevel1-gcc-ext.c
index d343f4e6e9cc837..32c872ffade1fd0 100644
--- a/libunwind/src/UnwindLevel1-gcc-ext.c
+++ b/libunwind/src/UnwindLevel1-gcc-ext.c
@@ -143,7 +143,7 @@ _Unwind_Backtrace(_Unwind_Trace_Fn callback, void *ref) {
   // Create a mock exception object for force unwinding.
   _Unwind_Exception ex;
   memset(&ex, '\0', sizeof(ex));
-  strcpy((char *)&ex.exception_class, "CLNGUNW");
+  memcpy(&ex.exception_class, "CLNGUNW", sizeof(ex.exception_class));
 #endif
 
   // walk each frame

[arichardson]

I would assume this is already transformed to memcpy but I guess it won't be for -O0

[arichardson]

Maybe use the string length here instead of the size of the exception class?

[michael-kenzel]

My reasoning was that if the string happened to be too long for some reason, we'd want to ensure that we don't write past the member. Using the string length would require duplicating the string literal, or introducing a constant, which would probably require a macro since this is C and so we can't just use constexpr (I doubt we can assume C23?) or some helper function/macro to wrap the copy. Either option, using the string length or member length, has the downside of not considering the other length. Ideally, we'd have a _Static_assert to ensure the sizes match. But that then puts us back to the problem of needing to duplicate the string literal. I couldn't find a way to put any such improvements into plain C without making this code significantly more complex. I was also not quite sure what versions of C this needs to build under, which would have implications regarding what features can be used.

One very simple solution (at least code-wise) would be to just do

  _Unwind_Exception ex = {
    .exception_class = 0x574E55474E4C43  // "CLNGUNW"
  };

That would take care of both the memset and the memcpy. But this then assumes little endian and so would be a potential ABI break on big-endian systems?

Overall, I think it's reasonable to assume that this string isn't just gonna change in a way that would make it not match the size of the member anymore. The current implementation was already broken in that case as well (at least if the string would happen to be longer). Unfortunately, no compiler seems to issue a warning if you make the string not match the size of the member.

[arichardson]

I agree this is fine as is. Assigning the integer directly will cause endian issues, so using memcpy is better.

If we want to be extra safe, we could #define the string value and add a static_assert().

[michael-kenzel]

I would assume this is already transformed to memcpy but I guess it won't be for -O0

I did some testing on this: it seems that gcc replaces the strcpy with a simple mov even under -O0, but not the memcpy. clang does the reverse: it replaces the memcpy with a mov even under -O0 but not the strcpy. So it's a bit of a wash (I didn't expect either compiler to transform any of these with -O0 tbqh). Though since libunwind is primarily meant to be used with LLVM, this change then arguably presents the tiniest codegen improvement. On either compiler, turning on optimizations (even just -O1) results in the exact same codegen from either version (as one would expect).

[michael-kenzel]

I found another use of the same string copying in tests/forceunwind.pass.cpp and updated that to match.

[ldionne]

This seems reasonable. I read the discussion about the size of the member but I think this is unlikely to change much -- plus I think the compiler should be able to diagnose in case the string becomes smaller and we'd be copying from a past-the-end location into the data structure: https://godbolt.org/z/qP736q5je

Clang doesn't warn right now but GCC does. I filed #72455 about that.