port: fix non-glibc desktop linux build [crashpad/crashpad : main]
Elly Fong-Jones (Gerrit)
Attention is currently required from: Elly Fong-Jones, Mark Mentovai.
Patch set 1:Commit-Queue +1
1 comment:
Patchset:
mark: please have a peek?
To view, visit change 4255370. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Elly Fong-Jones.
2 comments:
Patchset:
LGTM. I wouldn’t mind if you chatted me to tell me what special thing you might be doing with musl. I’m a libc lover.
Also, thanks for checking with other libc implementations like µClibc.
File util/linux/thread_info.h:
Patch Set #1, Line 277:
#if defined(__BIONIC__)Can you `#include <sys/cdefs.h>` before trying to use this macro?
To view, visit change 4255370. To unsubscribe, or for help writing mail filters, visit settings.
Elly Fong-Jones (Gerrit)
Attention is currently required from: Mark Mentovai.
2 comments:
Patchset:
LGTM. I wouldn’t mind if you chatted me to tell me what special thing you might be doing with musl. […]
Sure! I too am a libc carer-abouter - maybe we should catch up over VC some time.
My immediate project is reducing the set of patches that Alpine (which uses musl) has to apply to Chromium to get it to build; the dream is that they'll be able to just build upstream Chromium directly without needing a bunch of downstream patches. I'm not sure how feasible that is, but I've so far found a lot of relatively easy things to fix and only a few difficult ones.
File util/linux/thread_info.h:
Patch Set #1, Line 277:
#if defined(__BIONIC__)
Can you `#include <sys/cdefs. […]
I can, but musl doesn't have sys/cdefs.h, so I'd need to do something else to make the include itself conditional. The situation is actually a bit awkward because in bionic (only) sys/cdefs.h actually defines `__BIONIC__`, but in other libcs a header with the same name is instead responsible for defining a bunch of compat shims macros, like `__THROW`, `__restrict`, etc.
One option is to check for defined(ANDROID) instead of defined(`__BIONIC__`) here. We currently have `__BIONIC__` defined because bionic stdint.h happens to include sys/cdefs.h, but we could avoid using it. It feels a little weird / unclean to depend on that include, but it also feels weird to assume bionic if & only if android.
Would you prefer:
1. Current thing (my reluctant favorite)
2. Make the include conditional on build configs where sys/cdefs.h is known to exist
3. Change this conditional to ANDROID instead of `__BIONIC__`
4. Something else?
To view, visit change 4255370. To unsubscribe, or for help writing mail filters, visit settings.
Elly Fong-Jones (Gerrit)
Attention is currently required from: Mark Mentovai.
1 comment:
File util/linux/thread_info.h:
Patch Set #1, Line 277:
#if defined(__BIONIC__)
I can, but musl doesn't have sys/cdefs. […]
Oh - after a bit of further research, it seems like the header <feature.h> reliably both exists and defines the libc test macros like `__BIONIC__`. In particular, in bionic it pulls in sys/cdefs.h itself, and in musl/glibc/dietlibc/uclibc it defines the respective libc macros, as well as stuff like _ISO_C99.
To view, visit change 4255370. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Elly Fong-Jones.
Patch set 2:Code-Review +1
1 comment:
File util/linux/thread_info.h:
Patch Set #1, Line 277:
#if defined(__BIONIC__)
Oh - after a bit of further research, it seems like the header <feature.h> reliably both exists and defines the libc test macros like `__BIONIC__`. In particular, in bionic it pulls in sys/cdefs.h itself, and in musl/glibc/dietlibc/uclibc it defines the respective libc macros, as well as stuff like _ISO_C99.
Great! Let's do this.
To view, visit change 4255370. To unsubscribe, or for help writing mail filters, visit settings.
Elly Fong-Jones (Gerrit)
Attention is currently required from: Elly Fong-Jones.
Patch set 2:Commit-Queue +2
Crashpad LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted this change.
port: fix non-glibc desktop linux build
This is the only change needed to build crashpad against musl, yay! The
reason this change is needed is that user_vfp is bionic-specific, and
does not exist in glibc, dietlibc, uclibc, or musl.
I have not (yet) tried running the tests against another libc.
Bug: chromium:1380656
Change-Id: I2247352e1611a300dff995156d393508c8257039
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4255370
Reviewed-by: Mark Mentovai <ma...@chromium.org>
Commit-Queue: Elly Fong-Jones <elly...@chromium.org>
---
M util/linux/thread_info.h
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/util/linux/thread_info.h b/util/linux/thread_info.h
index 8b94eff..9f60bd3 100644
--- a/util/linux/thread_info.h
+++ b/util/linux/thread_info.h
@@ -15,6 +15,7 @@
#ifndef CRASHPAD_UTIL_LINUX_THREAD_INFO_H_
#define CRASHPAD_UTIL_LINUX_THREAD_INFO_H_
+#include <features.h>
#include <stdint.h>
#include <sys/user.h>
@@ -274,7 +275,7 @@
"Size mismatch");
#elif defined(ARCH_CPU_ARMEL)
static_assert(sizeof(f32_t::fpregs) == sizeof(user_fpregs), "Size mismatch");
-#if !defined(__GLIBC__)
+#if defined(__BIONIC__)
static_assert(sizeof(f32_t::vfp) == sizeof(user_vfp), "Size mismatch");
#endif
#elif defined(ARCH_CPU_ARM64)
To view, visit change 4255370. To unsubscribe, or for help writing mail filters, visit settings.