Clang's __builtin_return_address() currently sometimes returns a PAC signed pointer and sometimes not. This patch works around that by always ensuring that the pointer is not signed. rdar://71648468
Created attachment 417215 [details] proposed patch.
Comment on attachment 417215 [details] proposed patch. r=me
Comment on attachment 417215 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=417215&action=review > Source/WebKit/PluginProcess/mac/PluginProcessShim.mm:43 > +#if CPU(ARM64E) Is it even possible to make an arm64 plug-in that WebKit will load?
(In reply to Alexey Proskuryakov from comment #3) > Comment on attachment 417215 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=417215&action=review > > > Source/WebKit/PluginProcess/mac/PluginProcessShim.mm:43 > > +#if CPU(ARM64E) > > Is it even possible to make an arm64 plug-in that WebKit will load? I actually don't know.
Comment on attachment 417215 [details] proposed patch. Thanks for the review. Landing now.
Committed r271279: <https://trac.webkit.org/changeset/271279> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417215 [details].
Comment on attachment 417215 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=417215&action=review > Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:239 > +#if CPU(ARM64E) Surprised that this is #if CPU(ARM64E) and not something more like #if HAVE/USE(TAGGED_POINTERS). > Source/JavaScriptCore/interpreter/CallFrame.h:322 > +// FIXME (see rdar://72897291): Work around a Clang bug where __builtin_return_address() > +// sometimes gives us a signed pointer, and sometimes does not. Doesn't seem critical to have that comment here. It’s more valuable at the site of the workarounds.
Comment on attachment 417215 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=417215&action=review >> Source/JavaScriptCore/interpreter/CallFrame.h:322 >> +// sometimes gives us a signed pointer, and sometimes does not. > > Doesn't seem critical to have that comment here. It’s more valuable at the site of the workarounds. Right below this, we use a removeCodePtrTag() on the result of __builtin_return_address(). This shouldn't be needed if __builtin_return_address() actually behaves consistently. That's why I added this comment here to note this.
Comment on attachment 417215 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=417215&action=review >>> Source/JavaScriptCore/interpreter/CallFrame.h:322 >>> +// sometimes gives us a signed pointer, and sometimes does not. >> >> Doesn't seem critical to have that comment here. It’s more valuable at the site of the workarounds. > > Right below this, we use a removeCodePtrTag() on the result of __builtin_return_address(). This shouldn't be needed if __builtin_return_address() actually behaves consistently. That's why I added this comment here to note this. OK. I misunderstood because the removeCodePtrTag was already there. Just curious, why was it already there?
Comment on attachment 417215 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=417215&action=review >> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:239 >> +#if CPU(ARM64E) > > Surprised that this is #if CPU(ARM64E) and not something more like #if HAVE/USE(TAGGED_POINTERS). To me, CPU(ARM64E) is better since ARM64E ABI involves pointer tagging. If we introduce TAGGED_POINTERS, we need to consider about pair of TAGGED_POINTERS + ARM64, TAGGED_POINTERS + X64 etc. while they do not exist. So long as ARM64E is only one ABI using pointer tagging, I prefer using ARM64E here.
Comment on attachment 417215 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=417215&action=review >>> Source/JavaScriptCore/assembler/MacroAssemblerCodeRef.h:239 >>> +#if CPU(ARM64E) >> >> Surprised that this is #if CPU(ARM64E) and not something more like #if HAVE/USE(TAGGED_POINTERS). > > To me, CPU(ARM64E) is better since ARM64E ABI involves pointer tagging. If we introduce TAGGED_POINTERS, we need to consider about pair of TAGGED_POINTERS + ARM64, TAGGED_POINTERS + X64 etc. while they do not exist. > So long as ARM64E is only one ABI using pointer tagging, I prefer using ARM64E here. I don’t understand your logic at all!