Bug 206804 – Introduce a getVTablePointer() utility function.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

With getVTablePointer(), we can abstract away how we get a vtable function pointer without assuming the way it is signed for ARM64E.  With this, we can remove the WTF_PREPARE_VTBL_POINTER_FOR_INSPECTION macro which assumes how a vtable function pointer is signed.

<rdar://problem/58872290>

[Mark Lam]

Created attachment 388787 [details]
proposed patch.

[Oliver Hunt]

Comment on attachment 388787 [details]
proposed patch.

LGTM, though I am no longer a reviewer :D

[Yusuke Suzuki]

Comment on attachment 388787 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=388787&action=review

> Source/WTF/wtf/PointerPreparations.h:40
> +#if __has_builtin(__builtin_get_vtable_pointer)
> +#define getVTablePointer(o) __builtin_get_vtable_pointer(o)
> +#else
> +#define getVTablePointer(o) __builtin_ptrauth_auth(*(reinterpret_cast<void**>(o)), ptrauth_key_cxx_vtable_pointer, 0)
> +#endif

Is it supported in newer clang? This is out of this patch's scope, but I was considering that whether we can just use `__builtin_get_vtable_pointer` to determine a lot of C++ WebCore class types.

[Oliver Hunt]

Comment on attachment 388787 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=388787&action=review

>> Source/WTF/wtf/PointerPreparations.h:40
>> +#endif
> 
> Is it supported in newer clang? This is out of this patch's scope, but I was considering that whether we can just use `__builtin_get_vtable_pointer` to determine a lot of C++ WebCore class types.

You can't get metadata - so no sub typing checks, you could theoretically use it for exact matching but that doesn't work for MI (you only get the primary vtable), but the joy of linking could mean duplicate vtable definitions for some classes so it could only ever be a speculative optimization.

You *could* do a comparison to the destructor - e.g.

vtable[0] == &Class::~Class

[Mark Lam]

Thanks for the reviews.  Landed in r255126: <https://trac.webkit.org/r255126>.

[Darin Adler]

Comment on attachment 388787 [details]
proposed patch.

I think this patch goes against our WebKit coding style tradition of using normal casing for functions including inline functions, but using ALL_CAPS for macros, even function-like macros. But maybe we agreed on something different a while back? The most dangerous macros are the non-function-style ones, so maybe those are the ones we are strongly encouraging ALL_CAPS for?

[Mark Lam]

(In reply to Darin Adler from comment #6)
> Comment on attachment 388787 [details]
> proposed patch.
> 
> I think this patch goes against our WebKit coding style tradition of using
> normal casing for functions including inline functions, but using ALL_CAPS
> for macros, even function-like macros. But maybe we agreed on something
> different a while back? The most dangerous macros are the non-function-style
> ones, so maybe those are the ones we are strongly encouraging ALL_CAPS for?

You are right.  This totally slipped my mind.  Sorry about that.

I'll fix this by making getVTablePointer() into an inlined function.

[Mark Lam]

(In reply to Mark Lam from comment #7)
> I'll fix this by making getVTablePointer() into an inlined function.

Will address this in https://bugs.webkit.org/show_bug.cgi?id=206816.