[LLDB] Add ifndef to platform linux #141971
Conversation
|
@llvm/pr-subscribers-lldb Author: Jacob Lalonde (Jlalond) ChangesAnother iteration of fixes for #141670. Platform linux can be used by other platforms, so we need to supply the signal values if they're not defined. Full diff: https://github.com/llvm/llvm-project/pull/141971.diff 1 Files Affected:
diff --git a/lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp b/lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
index cb60caf1cb422..dd6490c7141e5 100644
--- a/lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
+++ b/lldb/source/Plugins/Platform/Linux/PlatformLinux.cpp
@@ -34,6 +34,20 @@
#define MAP_PRIVATE 2
#define MAP_ANON 0x20
+// For other platforms that use platform linux
+#ifndef SIGILL
+#define SIGILL 4
+#endif
+#ifndef SIGBUS
+#define SIGBUS 7
+#endif
+#ifndef SIGFPE
+#define SIGFPE 8
+#endif
+#ifndef SIGSEGV
+#define SIGSEGV 11
+#endif
+
using namespace lldb;
using namespace lldb_private;
using namespace lldb_private::platform_linux;
|
| @@ -34,6 +34,20 @@ | |||
| #define MAP_PRIVATE 2 | |||
| #define MAP_ANON 0x20 | |||
|
|
|||
| // For other platforms that use platform linux | |||
There was a problem hiding this comment.
Are these guaranteed to be defines and not constants?
| #define SIGFPE 8 | ||
| #endif | ||
| #ifndef SIGSEGV | ||
| #define SIGSEGV 11 |
There was a problem hiding this comment.
You mentioned offline that we define these somewhere else as well? Would be good to have a cleanup to unify them into a single header I think.
There was a problem hiding this comment.
They're all defined in signal.h, but we can build this class on a platform that won't have access to signal.h, so we need to ensure they're defined.
|
I'm going to wait to see how many build issues we get before landing. |
|
Ummm... I should have caught this before, but this isn't correct because the host system may (and some do) use different signal numbers for these constants. So, on windows, you get an undefined symbol error, but on e.g. macos, you'd just get a wrong value. You should take these values from the UnixSignals object. |
Are these actually different on Mac? These 4 are the same on BSD to my knowledge. How do you think we should go about getting the value from UnixSignals? Edit: BSD Man page, same values for We might want to make a follow up to clean this up but this should unblock the build. I'm not sure how Linux Signals is building actually, but it appears to have built successfully |
|
@labath I'm landing to not leave the build broken, but if you have a better solution/means of doing this let me know and I'll implement it |
|
https://lab.llvm.org/buildbot/#/builders/197/builds/6014 looks like we're passing |
I haven't checked, I guess these four might be the same on a mac, but that definitely is not the case everywhere. We're currently growing AIX support, and that one seems to have different numbers: https://www.ibm.com/docs/en/sdk-java-technology/8?topic=reference-signal-handling. According to the linux manpage, the values can vary even across linux different architectures.
Uses some macro trickery. We provide the symbolic constant and the value. We always use the constant value, but on linux, the macro also expands to a static_assert which checks that our constant matches what the system headers say.
You should be able to get the UnixSignals object from the thread ( |
Another iteration of fixes for #141670. Platform linux can be used by other platforms, so we need to supply the signal values if they're not defined. Values are from the [manpage](https://man7.org/linux/man-pages/man7/signal.7.html)
Another iteration of fixes for llvm#141670. Platform linux can be used by other platforms, so we need to supply the signal values if they're not defined. Values are from the [manpage](https://man7.org/linux/man-pages/man7/signal.7.html)
Another iteration of fixes for llvm#141670. Platform linux can be used by other platforms, so we need to supply the signal values if they're not defined. Values are from the [manpage](https://man7.org/linux/man-pages/man7/signal.7.html)
Another iteration of fixes for #141670. Platform linux can be used by other platforms, so we need to supply the signal values if they're not defined.
Values are from the manpage