Using $next in conditional fields generates incorrect offsets

[afoxley]

I'm writing a definition for a protocol where the length field is dynamically sized. I've noted that all the fields after the conditional that pulls the extended length portion out are pointing at the wrong offset. It seems that $next is not doing what one would expect in the presence of a conditional.

Definition:

enum RfcommFrameType:
  [maximum_bits: 8]
  SET_ASYNC_BALANCED_MODE = 0x2f
  UNNUMBERED_ACK = 0x63
  DISCONNECT_MODE = 0x0f
  DISCONNECT = 0xc3
  UNNUMBERED_INFORMATION_WITH_HEADER_CHECK = 0xef
  UNNUMBERED_INFORMATION_WITH_HEADER_CHECK_AND_POLL_FINAL = 0xff
    -- RFCOMM extension. With Poll/Final bit set in UIH control byte, a credits
    -- field is the first byte of the payload.

enum RfcommFixedChannel:
  [maximum_bits: 8]
  CONTROL = 0

enum RfcommCommandResponse:
  -- Commands from the initiator and responses from the responder
  -- have C/R = 0. Commands from the responder and responses from
  -- the initiator have C/R = 1.
  [maximum_bits: 1]
  COMMAND  = 0
  RESPONSE = 1

enum RfcommDirection:
  [maximum_bits: 1]
  RESPONDER = 0
  INITIATOR = 1

enum RfcommLengthExtended:
  [maximum_bits: 1]
  NORMAL = 1
  EXTENDED = 0

struct RfcommFrame:
  -- RFCOMM TS 07.10 frame
  [requires: extended_address == true]
  0    [+1]  bits:
    0  [+1]  Flag                   extended_address
    1  [+1]  RfcommCommandResponse  command_response
    2  [+1]  RfcommDirection        direction
    3  [+5]  UInt                   channel

  1    [+1]  RfcommFrameType        control

  2    [+1]  bits:
    0  [+1]  RfcommLengthExtended   length_extended_flag
    1  [+7]  UInt                   length

  if length_extended_flag == RfcommLengthExtended.EXTENDED:
    $next  [+1]                UInt                     length_extended

  if control == RfcommFrameType.UNNUMBERED_INFORMATION_WITH_HEADER_CHECK_AND_POLL_FINAL && channel != 0:
    $next  [+1]                UInt                     credits
      -- Credits field can appears as first byte of payload when Poll/Final bit is
      -- set on UIH frames and channel is not control (0).

  let combined_length = $present(length_extended) ? length * 256 + length_extended : length

  # If present, credits is counted as part of payload.
  let payload_length = $present(credits) ? combined_length - 1 : combined_length

  $next  [+payload_length]           UInt:8[payload_length]           payload

  $next  [+1]  UInt                   fcs
    -- Frame checksum. See: GSM 07.10 TS 101 369 V6.3.0
    -- SABM, DISC, UA, DM frame types:
    --   FCS should be calculated over address, control and length fields.
    -- UIH frame type:
    --   FCS should be calculated over address and control fields.

Everything after

  if length_extended_flag == RfcommLengthExtended.EXTENDED:
    $next  [+1]                UInt                     length_extended

is pointing one byte off, even if the length_extended_flag != RfcommLengthExtended.EXTENDED. I confirmed this with a !has_length_extended() check and looking at the emboss_reserved_local_offset variable in the generated code for each field.

For example one would expect credits to have offset 3 when length_extended_flag != RfcommLengthExtended.EXTENDED, but it actually has 4.

On a related note: IntrinsicSizeInBytes().Read() reports one more byte than I would expect on this struct as well. Like it isn't taking into account the conditionals correctly.

There's some messy code that uses the generated stuff here for more context https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/246974

[afoxley]

Confirming the problem seems to be with $next, as manually calculating the offsets works:

  if control == RfcommFrameType.UNNUMBERED_INFORMATION_WITH_HEADER_CHECK_AND_POLL_FINAL && channel != 0:
    let credits_offset = $present(length_extended) ? 4 : 3
    credits_offset  [+1]                UInt                     credits
      -- Credits field can appears as first byte of payload when Poll/Final bit is
      -- set on UIH frames and channel is not control (0).

  let combined_length = $present(length_extended) ? length * 256 + length_extended : length

  # If present, credits is counted as part of payload.
  let payload_length = $present(credits) ? combined_length - 1 : combined_length

  let payload_offset = $present(credits) ? credits_offset + 1 : credits_offset
  payload_offset  [+payload_length]           UInt:8[payload_length]           payload

  let fcs_offset = payload_offset + payload_length
  fcs_offset  [+1]  UInt                   fcs

[afoxley]

Maybe related to #191 @jasongraffius

[reventlov]

Yes, this is #191. It's more of a design bug than an implementation bug, and there is a design proposal to fix it in PR #192.

[afoxley]

I'm not the first to make this mistake with $next looks like, so I do think it should be explicitly called out in the docs.