Bug 220432

Summary: Work around Clang bug in __builtin_return_address().
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, keith_miller, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch. none

Description Mark Lam 2021-01-07 13:00:15 PST 
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
Comment 1 Mark Lam 2021-01-07 15:03:38 PST 
Created attachment 417215 [details]
proposed patch.
Comment 2 Yusuke Suzuki 2021-01-07 15:39:00 PST 
Comment on attachment 417215 [details]
proposed patch.

r=me
Comment 3 Alexey Proskuryakov 2021-01-07 15:42:47 PST 
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?
Comment 4 Mark Lam 2021-01-07 16:10:59 PST 
(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 5 Mark Lam 2021-01-07 17:12:07 PST 
Comment on attachment 417215 [details]
proposed patch.

Thanks for the review.  Landing now.
Comment 6 EWS 2021-01-07 17:36:51 PST 
Committed r271279: <https://trac.webkit.org/changeset/271279>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417215 [details].
Comment 7 Darin Adler 2021-01-08 11:31:04 PST 
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 8 Mark Lam 2021-01-08 11:48:10 PST 
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 9 Darin Adler 2021-01-08 12:16:15 PST 
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 10 Yusuke Suzuki 2021-01-08 12:29:14 PST 
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 11 Darin Adler 2021-01-08 12:36:49 PST 
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!