[iOS arm64e] Fix base/profiler to work correctly with arm64e [chromium/src : main]
Justin Novosad (Gerrit)
Justin Novosad added 5 comments![]( "Open in Gerrit")
#include <ptrauth.h>Justin NovosadWill this really work unguarded in all environments?
As far as I can tell this should be perfectly safe. In ptrauth.h there's an `#else` clause that defines no-op versions of all the intrinsic wrappers whenever ptrauth is not supported. Also, CQ checks pass.
#if BUILDFLAG(ARCH_CPU_PTRAUTH)
#define __sp __opaque_sp
#define __fp __opaque_fp
#define __pc __opaque_pc
#endif // BUILDFLAG(ARCH_CPU_PTRAUTH)Justin NovosadI don’t love it: these may already have been `#define`d as macros, and the `#undef` later on undoes that. Double-underscore names really aren’t ours to alter. Can you find a different strategy?
Done
#if defined(ARCH_CPU_ARM_FAMILY) && defined(ARCH_CPU_32_BITS)Justin NovosadThese cascades are about CPU type, not CPU family. It’s kind of backwards to tie the CPU family and bitness together to get the CPU type, when we already have `ARCH_CPU_*` macros without the `*_FAMILY` to fit the bill: `ARCH_CPU_ARMEL`, `ARCH_CPU_ARM64`, `ARCH_CPU_X86`, `ARCH_CPU_X86_64`.
Same in other functions.
Done
#endifJustin Novosad```
#else
#error port
```because these void-returning functions otherwise give no indication that they’re accidental no-ops if not ported to new architectures.
Same in the two other setter functions.
This logic is not new to this CL. If I'm reading it correctly there's no risk of accidental no-op here. The final `#else` assumes X86 (32 bit). If that assumption is wrong, compilation will fail because Esp will be undefined. Anyways, explicitly checking for X86 with a fallback that emits an error makes this clearer. Fixed.
#if defined(ARCH_CPU_PTRAUTH)
AsUintPtr(&context->__opaque_sp) = val;
#else // !defined(ARCH_CPU_PTRAUTH)
context->__sp = val;
#endif // !defined(ARCH_CPU_PTRAUTH)Justin NovosadSame: cane these be simplified? A common function that inserts the __opaque by string pasting in ptrauth mode?
Justin NovosadBy the time I got to addressing this comment, I had already simplified this code in response to Sylvain's comments. I used macros to inject the `opaque` prefix. WDYT?
Done
Related details
- Dave Tapuska
- Mark Mentovai
- Sean Maher
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
1 comment
#include <ptrauth.h>Justin NovosadWill this really work unguarded in all environments?
As far as I can tell this should be perfectly safe. In ptrauth.h there's an `#else` clause that defines no-op versions of all the intrinsic wrappers whenever ptrauth is not supported. Also, CQ checks pass.
As far as I can tell this should be perfectly safe. In ptrauth.h there's an `#else` clause that defines no-op versions of all the intrinsic wrappers whenever ptrauth is not supported. Also, CQ checks pass.
Ah, Clang always provides its own `<ptrauth.h>`. OK.
I expect this to be a spot that will need further attention in order for this code to build with other toolchains. But as long as our bots like it, it’s fine to kick that can down the road (and to the people who will be affected). We (they) can deal with it when it happens.
Related details
- Dave Tapuska
- Justin Novosad
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Novosad (Gerrit)
Justin Novosad voted Commit-Queue+2![]( "Open in Gerrit")
- Dave Tapuska
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sylvain Defresne
| Code-Review | +1 |
1 comment
Related details
- Dave Tapuska
- Justin Novosad
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[iOS arm64e] Fix base/profiler to work correctly with arm64e
With this CL, it is now possible to build base_unittests for arm64e.
Some of the tests still crash due to missing explicit pointer
authentication handling in low level routines and assembly code.
Access to pointer registers in the thread state had to be refactored to
use getter and setter methods (instead of readwrite access via
references) so that opaque pointers can be encoded and decoded correctly
using arm64e pointer authentication.
There are no automated tests that cover the arm64e builds at this
time because the build is not yet functional. However, it is
possible to manually test this CL using the following steps:
* In a Bling checkout, set the target_cpu gn arg to "arm64e" in
a build configuration that is otherwise set up to build for
a physical device (e.g. out/Debug-iphoneos)
* Build the base_unittests target.
Note: much of Chromium won't currently build correctly in arm64e,
but base unittests does, as of this CL. Because rust does not
support arm64e, you will see linker warnings about rust deps being
ignored. This is fine, you will still be able to build and run
the tests and it will still include the unit tests affected by
this CL.
* Run base_unittests on a physical device. To get around tests that
still crash, use the --gtest_filter command line arg to successfully
run the following test cases, which are relevant to this CL:
StackCopierTest, StackCopierSuspendTest, StackBufferTest,
StackTraceTest, StackTraceDeathTest, FramePointerUnwinder,
Low-Coverage-Reason: COVERAGE_UNDERREPORTED The gaps in coverage
signalled by the code coverage checks are in areas of the code that are
specific to certain CPU architectures. These gaps due to a combination
of certain tests being disabled on linux and not all runs of
base_unittests being tracked for code coverage analysis.
- M base/debug/stack_trace_unittest.cc
- M base/profiler/chrome_unwinder_android_32.cc
- M base/profiler/chrome_unwinder_android_32_unittest.cc
- M base/profiler/frame_pointer_unwinder.cc
- M base/profiler/frame_pointer_unwinder_unittest.cc
- M base/profiler/register_context_registers.h
- M base/profiler/stack_copier.cc
- M base/profiler/stack_copier.h
- M base/profiler/stack_copier_signal.cc
- M base/profiler/stack_copier_signal.h
- M base/profiler/stack_copier_suspend.cc
- M base/profiler/stack_copier_suspend.h
- M base/profiler/stack_copier_suspend_unittest.cc
- M base/profiler/stack_copier_unittest.cc
- M base/profiler/stack_sampler_unittest.cc
- M base/profiler/suspendable_thread_delegate_mac.cc
- M base/profiler/suspendable_thread_delegate_mac.h
- M base/profiler/suspendable_thread_delegate_win.cc
- M base/profiler/suspendable_thread_delegate_win.h
- M base/profiler/thread_delegate.h
- M base/profiler/thread_delegate_posix.cc
- M base/profiler/thread_delegate_posix.h
- M build/build_config.h
- M chrome/renderer/v8_unwinder.cc
Code-Review: +1 by Mark Mentovai, +1 by Sylvain Defresne
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
luci-bisection@appspot.gserviceaccount.com (Gerrit)
Message from luci-bi...@appspot.gserviceaccount.com![]( "Open in Gerrit")
LUCI Bisection has identified this change as the culprit of a build failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/compile-analysis/b/8684344813737616305
A revert for this change was not created because there are merged changes depending on it.
Sample failed build: https://ci.chromium.org/b/8684344813737616305
If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F7708137&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Fcompile-analysis%2Fb%2F8684344813737616305&type=BUG
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mohannad Farrag (Gerrit)
Mohannad Farrag has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |