Bug 220432 – Work around Clang bug in __builtin_return_address().

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

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

[Mark Lam]

Created attachment 417215 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 417215 [details]
proposed patch.

r=me

[Alexey Proskuryakov]

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?

[Mark Lam]

(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.

[Mark Lam]

Comment on attachment 417215 [details]
proposed patch.

Thanks for the review.  Landing now.

[EWS]

Committed r271279: <https://trac.webkit.org/changeset/271279>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417215 [details].

[Darin Adler]

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.

[Mark Lam]

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.

[Darin Adler]

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?

[Yusuke Suzuki]

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.

[Darin Adler]

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!