Bug 220432 - Work around Clang bug in __builtin_return_address().
Summary: Work around Clang bug in __builtin_return_address().
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-07 13:00 PST by Mark Lam
Modified: 2021-01-08 12:36 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (19.63 KB, patch)
2021-01-07 15:03 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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!