-
Notifications
You must be signed in to change notification settings - Fork 14.9k
libunwind: Implement the unw_strerror function for better nongnu libunwind compatibility #160887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@llvm/pr-subscribers-libunwind Author: Gleb Popov (arrowd) ChangesAs it was explained to me in https://discourse.llvm.org/t/libunwinds-raison-detre/88283/2 the LLVM version of libunwind is mostly compatible with nongnu one. This change improves the compatibility a bit further. Full diff: https://github.com/llvm/llvm-project/pull/160887.diff 3 Files Affected:
diff --git a/libunwind/include/libunwind.h b/libunwind/include/libunwind.h
index 94928f436025a..9d2ccc4dda7fa 100644
--- a/libunwind/include/libunwind.h
+++ b/libunwind/include/libunwind.h
@@ -130,6 +130,7 @@ extern int unw_is_fpreg(unw_cursor_t *, unw_regnum_t) LIBUNWIND_AVAIL;
extern int unw_is_signal_frame(unw_cursor_t *) LIBUNWIND_AVAIL;
extern int unw_get_proc_name(unw_cursor_t *, char *, size_t, unw_word_t *) LIBUNWIND_AVAIL;
//extern int unw_get_save_loc(unw_cursor_t*, int, unw_save_loc_t*);
+extern const char *unw_strerror (int) LIBUNWIND_AVAIL;
extern unw_addr_space_t unw_local_addr_space;
diff --git a/libunwind/src/libunwind.cpp b/libunwind/src/libunwind.cpp
index cf39ec5f7dbdf..23335ba577e52 100644
--- a/libunwind/src/libunwind.cpp
+++ b/libunwind/src/libunwind.cpp
@@ -347,6 +347,29 @@ void __unw_remove_dynamic_eh_frame_section(unw_word_t eh_frame_start) {
}
#endif // defined(_LIBUNWIND_SUPPORT_DWARF_UNWIND)
+
+/// Maps the UNW_* error code to a textual representation
+_LIBUNWIND_HIDDEN const char *__unw_strerror(int error_code) {
+ switch (error_code) {
+ case UNW_ESUCCESS: return "no error";
+ case UNW_EUNSPEC: return "unspecified (general) error";
+ case UNW_ENOMEM: return "out of memory";
+ case UNW_EBADREG: return "bad register number";
+ case UNW_EREADONLYREG: return "attempt to write read-only register";
+ case UNW_ESTOPUNWIND: return "stop unwinding";
+ case UNW_EINVALIDIP: return "invalid IP";
+ case UNW_EBADFRAME: return "bad frame";
+ case UNW_EINVAL: return "unsupported operation or bad value";
+ case UNW_EBADVERSION: return "unwind info has unsupported version";
+ case UNW_ENOINFO: return "no unwind info found";
+#if defined(_LIBUNWIND_TARGET_AARCH64) && !defined(_LIBUNWIND_IS_NATIVE_ONLY)
+ case UNW_ECROSSRASIGNING: return "return address require authentication";
+#endif
+ }
+ return "invalid error code";
+}
+_LIBUNWIND_WEAK_ALIAS(__unw_strerror, unw_strerror)
+
#endif // !defined(__USING_SJLJ_EXCEPTIONS__) && !defined(__wasm__)
#ifdef __APPLE__
diff --git a/libunwind/src/libunwind_ext.h b/libunwind/src/libunwind_ext.h
index 28db43a4f6eef..ed503ceb70c5a 100644
--- a/libunwind/src/libunwind_ext.h
+++ b/libunwind/src/libunwind_ext.h
@@ -42,6 +42,7 @@ extern int __unw_get_proc_info(unw_cursor_t *, unw_proc_info_t *);
extern int __unw_is_fpreg(unw_cursor_t *, unw_regnum_t);
extern int __unw_is_signal_frame(unw_cursor_t *);
extern int __unw_get_proc_name(unw_cursor_t *, char *, size_t, unw_word_t *);
+extern const char *__unw_strerror(int);
#if defined(_AIX)
extern uintptr_t __unw_get_data_rel_base(unw_cursor_t *);
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…nwind compatibility
f892211
to
7652923
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
mind adding a test? you could probably piggyback on |
Turns out it isn't possible. Would you like me to create a separate testcase or let's just skip it? |
return "no unwind info found"; | ||
#if defined(_LIBUNWIND_TARGET_AARCH64) && !defined(_LIBUNWIND_IS_NATIVE_ONLY) | ||
case UNW_ECROSSRASIGNING: | ||
return "return address require authentication"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't grammatically correct. Was it copied verbatim as the error message used by another libunwind implementation or did you introduce this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, English isn't my native. I took message strings from nongnu libunwind: https://github.com/libunwind/libunwind/blob/bfc0d618f72ba4b725c3735188e0a6d8c710411c/src/mi/strerror.c#L31
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot just lift code verbatim from another project with a different license.
I beg my pardon, why did you close this? Reformulating string constants is not an option? |
The other libunwind has a different license. Copying code from that one into this one is not something we should do: #160887 (comment) |
All right, and once again, why did you close the PR? Is it rejected? I shouldn't try to fix raised comments and improve it? |
There seems to be some confusion here. As far as I can tell (and Gleb, please give explicit confirmation as to whether this is 100% accurate or not), no code has been copied (the variable names and formatting are different, it uses early return rather than variable assignment, and this even has the default case after the end of the switch rather than inside it), only the English text used for most of the error messages (possibly all but the arm64 PAC one?). |
That's right and it is pretty obvious from the diff.
It also has a LLVM-specific
Yes, I copied string constants from nongnu libunwind, mainly because I'm unsure if there is software in the wild that depends on them. Of course, I don't mind reformulating them. |
I deliberately didn't talk about that in this part of my message because it's not relevant; adding code does not affect whether the rest of the code is a license violation. |
Yes, but it is also a sort of evidence that I didn't just blindly copy the nongnu code. |
Not really. If you'd copied the nongnu code verbatim and added a single |
I don't think this little detail is worth arguing. What's more important (at least to me) is why the PR is closed and if it is now rejected or still has a chance to get merged. |
I closed it out of an abundance of caution because you wrote:
I want to make it clear that we CANNOT copy code out of another implementation with another license. |
I think it is okay to implement this interface, but doing so by lifting details out of another implementation is not fine. |
I'm not convinced that reusing the error strings counts as some form of copyright violation especially since the same value may in theory be required for API compatibility and therefore should be exempt. But I'm not a copyright lawyer. |
I just remembered that these strings are already present in the LLVM codebase: llvm-project/libunwind/include/libunwind.h Lines 54 to 67 in a4015d9
|
As it was explained to me in https://discourse.llvm.org/t/libunwinds-raison-detre/88283/2 the LLVM version of libunwind is mostly compatible with nongnu one. This change improves the compatibility a bit further.