Skip to content

Commit

Permalink
Have handleAppWrite and ignoreEarlyAppWrite take in parameter by refe…
Browse files Browse the repository at this point in the history
…rence

Summary:
Prior to this diff, the `EarlyAppWrite` was taken in by value, which meant that
there were 6 unique event handlers that needed to perform the allocate
stack/move/call/cleanup/exception handling for each call to `handleAppWrite` or
`ignoreEarlyAppWrite`.

This diff changes the signature to take the parameter by reference. This means
that w.r.t the caller, there are no locally scoped variables with non-trivial
destructors, so that the EventHandlers can simply tail call into the function.

Before:
```
0000000000039774 <_ZN4fizz2sm12EventHandlerINS_6client11ClientTypesELNS2_9StateEnumE1ELNS_5EventE16EE6handleERKNS2_5StateERNS_5ParamE>:
   39774:       d10143ff        sub     sp, sp, #0x50
   39778:       f9001bfe        str     x30, [sp, #48]
   3977c:       a9044ff4        stp     x20, x19, [sp, #64]
   39780:       d53bd053        mrs     x19, tpidr_el0
   39784:       910003f4        mov     x20, sp
   39788:       f9401669        ldr     x9, [x19, #40]
   3978c:       f90017e9        str     x9, [sp, #40]
   39790:       b9412029        ldr     w9, [x1, #288]
   39794:       71004d3f        cmp     w9, #0x13
   39798:       9a9f0029        csel    x9, x1, xzr, eq  // eq = none
   3979c:       3dc00120        ldr     q0, [x9]
   397a0:       f900053f        str     xzr, [x9, #8]
   397a4:       f940092a        ldr     x10, [x9, #16]
   397a8:       b9401929        ldr     w9, [x9, #24]
   397ac:       3d8003e0        str     q0, [sp]
   397b0:       f9000bea        str     x10, [sp, #16]
   397b4:       b9001be9        str     w9, [sp, #24]
   397b8:       910003e1        mov     x1, sp
   397bc:       94000011        bl      39800 <_ZN4fizz2smL19handleEarlyAppWriteERKNS_6client5StateENS_13EarlyAppWriteE>
   397c0:       b27d0280        orr     x0, x20, #0x8
   397c4:       97ffb75a        bl      2752c <_ZNSt6__ndk110unique_ptrIN5folly5IOBufENS_14default_deleteIS2_EEED2Ev>
   397c8:       f9401668        ldr     x8, [x19, #40]
   397cc:       f94017e9        ldr     x9, [sp, #40]
   397d0:       eb09011f        cmp     x8, x9
   397d4:       540000a1        b.ne    397e8 <_ZN4fizz2sm12EventHandlerINS_6client11ClientTypesELNS2_9StateEnumE1ELNS_5EventE16EE6handleERKNS2_5StateERNS_5ParamE+0x74>  // b.any
   397d8:       a9444ff4        ldp     x20, x19, [sp, #64]
   397dc:       f9401bfe        ldr     x30, [sp, #48]
   397e0:       910143ff        add     sp, sp, #0x50
   397e4:       d65f03c0        ret
   397e8:       9400770e        bl      57420 <__stack_chk_fail@plt>
   397ec:       aa0003f3        mov     x19, x0
   397f0:       b27d0280        orr     x0, x20, #0x8
   397f4:       97ffb74e        bl      2752c <_ZNSt6__ndk110unique_ptrIN5folly5IOBufENS_14default_deleteIS2_EEED2Ev>
   397f8:       aa1303e0        mov     x0, x19
   397fc:       940078d1        bl      57b40 <__wrap__Unwind_Resume@plt>

```

After:
```
0000000000039714 <_ZN4fizz2sm12EventHandlerINS_6client11ClientTypesELNS2_9StateEnumE1ELNS_5EventE16EE6handleERKNS2_5StateERNS_5ParamE>:
   39714:       14000001        b       39718 <_ZN4fizz2smL19handleEarlyAppWriteERKNS_6client5StateERNS_5ParamE>
```

Reviewed By: zalecodez

Differential Revision: D58164235

fbshipit-source-id: 9d06f5bf7f1a2c931131597f53ef6c3cd734fe46
  • Loading branch information
Mingtao Yang authored and facebook-github-bot committed Jun 6, 2024
1 parent 870db84 commit 7a1b1cb
Showing 1 changed file with 12 additions and 10 deletions.
22 changes: 12 additions & 10 deletions fizz/client/ClientProtocol.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2434,7 +2434,8 @@ EventHandler<ClientTypes, StateEnum::Established, Event::KeyUpdate>::handle(
// action to invoke depends on how the higher layer will react to rejected
// early data, we give the write back in a special action for the higher layer
// to handle.
static Actions ignoreEarlyAppWrite(const State& state, EarlyAppWrite write) {
static Actions ignoreEarlyAppWrite(const State& state, Param& param) {
auto& write = *param.asEarlyAppWrite();
if (*state.earlyDataType() != EarlyDataType::Rejected) {
throw FizzException("ignoring valid early write", folly::none);
}
Expand All @@ -2444,7 +2445,8 @@ static Actions ignoreEarlyAppWrite(const State& state, EarlyAppWrite write) {
return actions(std::move(failedWrite));
}

static Actions handleEarlyAppWrite(const State& state, EarlyAppWrite appWrite) {
static Actions handleEarlyAppWrite(const State& state, Param& param) {
auto& appWrite = *param.asEarlyAppWrite();
if (state.context()->getOmitEarlyRecordLayer()) {
throw FizzException("early app writes disabled", folly::none);
}
Expand All @@ -2453,7 +2455,7 @@ static Actions handleEarlyAppWrite(const State& state, EarlyAppWrite appWrite) {
case EarlyDataType::NotAttempted:
throw FizzException("invalid early write", folly::none);
case EarlyDataType::Rejected:
return ignoreEarlyAppWrite(state, std::move(appWrite));
return ignoreEarlyAppWrite(state, param);
case EarlyDataType::Attempted:
case EarlyDataType::Accepted: {
WriteToSocket write;
Expand Down Expand Up @@ -2486,42 +2488,42 @@ Actions EventHandler<
ClientTypes,
StateEnum::ExpectingServerHello,
Event::EarlyAppWrite>::handle(const State& state, Param& param) {
return handleEarlyAppWrite(state, std::move(*param.asEarlyAppWrite()));
return handleEarlyAppWrite(state, param);
}

Actions EventHandler<
ClientTypes,
StateEnum::ExpectingEncryptedExtensions,
Event::EarlyAppWrite>::handle(const State& state, Param& param) {
return handleEarlyAppWrite(state, std::move(*param.asEarlyAppWrite()));
return handleEarlyAppWrite(state, param);
}

Actions EventHandler<
ClientTypes,
StateEnum::ExpectingCertificate,
Event::EarlyAppWrite>::handle(const State& state, Param& param) {
return ignoreEarlyAppWrite(state, std::move(*param.asEarlyAppWrite()));
return ignoreEarlyAppWrite(state, param);
}

Actions EventHandler<
ClientTypes,
StateEnum::ExpectingCertificateVerify,
Event::EarlyAppWrite>::handle(const State& state, Param& param) {
return ignoreEarlyAppWrite(state, std::move(*param.asEarlyAppWrite()));
return ignoreEarlyAppWrite(state, param);
}

Actions
EventHandler<ClientTypes, StateEnum::ExpectingFinished, Event::EarlyAppWrite>::
handle(const State& state, Param& param) {
return handleEarlyAppWrite(state, std::move(*param.asEarlyAppWrite()));
return handleEarlyAppWrite(state, param);
}

Actions
EventHandler<ClientTypes, StateEnum::Established, Event::EarlyAppWrite>::handle(
const State& state,
Param& param) {
auto appWrite = std::move(*param.asEarlyAppWrite());
if (*state.earlyDataType() == EarlyDataType::Accepted) {
auto appWrite = std::move(*param.asEarlyAppWrite());
// It's possible that we had queued early writes before full handshake
// success. It's fine to write them on the normal record layer as long as
// the early data was accepted, otherwise we need to ignore them to preserve
Expand All @@ -2533,7 +2535,7 @@ EventHandler<ClientTypes, StateEnum::Established, Event::EarlyAppWrite>::handle(
write.flags = appWrite.flags;
return actions(std::move(write));
} else {
return ignoreEarlyAppWrite(state, std::move(appWrite));
return ignoreEarlyAppWrite(state, param);
}
}

Expand Down

0 comments on commit 7a1b1cb

Please sign in to comment.