RFR: 8287325: AArch64: fix virtual threads with -XX:UseBranchProtection=pac-ret [v6]

[Hao Sun]

On Mon, 11 Sep 2023 09:35:19 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> I doubt that.
>> 
>> In current design, we declare member functions in the shared `ContinuationHelper.hpp`, and define them in the architecture specific `ContinuationHelper_xx.inline.hpp` (e.g, `ContinuationHelper_aarch64.inline.hpp` for AArch64 backend).
>> 
>> Following current design, if we introduce one base class(e.g., `class ContinuationCommonHelper`) and the default `patch_return_address_at()` implementation, we still have to declare `ContinuationHelper::patch_return_address_at()` in the shared `ContinuationHelper.hpp` and define it for all architectures inevitably.
>> 
>> Otherwise, we have to 
>> 1) only declare `ContinuationHelper::patch_return_address_at()` in `ContinuationHelper.hpp` for AArch64 target with the help of conditional compilation directives. OR
>> 2) put the declaration of `class ContinuationHelper` to `ContinuationHelper_xx.inline.hpp` and only overwride `patch_return_address_at()` for AArch64.
>> 
>> But I think neither way is neat. WDYT? Thanks.
>
> Why not define the default `BaseContinuationHelper::patch_return_address_at()` in ContinuationHelper.hpp?

I guess the following pseudo-code is what you want:


/*
 *  file ContinuationHelper.hpp
 */

class BaseContinuationHelper {
  public:
    inline void patch_return_address_at(intptr_t* sp, address pc) {
      // the default implementation
    }
}

class ContinuationHelper : public BaseContinuationHelper {
  public: 
    inline void patch_return_address_at(intptr_t* sp, address pc) {} // declare here
}

/*
 *  file ContinuationHelper_aarch64.inline.hpp
 */

inline void ContinuationHelper::patch_return_address_at(intptr_t* sp, address pc) {
  // override here for AArch64
}

/*
 *  file ContinuationHelper_x86.inline.hpp
 */

// no need to define patch_return_address_at().
// use the default BaseContinuationHelper::patch_return_address_at().


However, it doesn't work because we have to define `ContinuationHelper::patch_return_address_at()` for x86 since we declare it.

Please let me know if I misunderstood something.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13322#discussion_r1321300062