Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix negative immediate offset handling for JUMPS and JUMPR opcodes #18

Merged
merged 2 commits into from
Sep 21, 2021

Conversation

wnienhaus
Copy link
Contributor

@wnienhaus wnienhaus commented Aug 20, 2021

When providing negative immediate offset (step) values to the JUMPR and JUMPS opcodes, the resulting instruction contained an incorrect offset.

This PR fixes that behaviour.

According to the ESP32 Technical Reference Manual (TRM) the magnitude of the relative shift from the current position is determined by bits 0-6 of the step field, and the direction is determined by bit 7, with 0 meaning PC+step, and 1 meaning PC-step.

(For comparison, the ULP C macros in the ESP-IDF implement this correctly, as described in the TRM (code). All step values passed to the relevant JUMP macros will result in the instruction step field having bit 7 indicating the sign and bits 0-6 holding the absolute value of the offset.)

This PR modifies the I_JUMP_REL{R,S} macros to set the step field correctly for negative immediate values. Since symbols, which are resolved later during the BFD relocation phase, always evaluate to 0 at this stage (from EXPR_VALUE), this change only affects the case of immediate values (as can be seen from all previous test cases resulting in the same listing output as before). For symbols, the relocation code (in function esp32ulp_jumprelr_reloc) already did this correctly, and thus that code remains unchanged.

Example of the issue:

For an offset of -2, the step field should have looked as follows:

  bit 0-6 = 2   # positive 2
  bit   7 = 1   # 1 means negative

However, the result was actually:

  bit 0-6 = 126  # negative 2 (two's compliment)
  bit   7 = 1    # 1 means negative

ESP32-S2

The PR also fixes this (as a separate commit) for the ES32-S2 implementation of the same opcodes.

However, for the ESP32-S2 it appears there are three more issues (which this PR does not address):

  1. for the JUMPS opcode, the step value is not divided by 4 (step_val>>2), even though the documentation states that step is expressed in 32-bit words. (see this code, that should probably use step_val>>2 rather than step_val)
  2. the ESP32-S2 Technical Reference Manual incorrectly shows the Cond field of the JUMPS instruction as being 2 bit wide, while the code uses 3 bits for this field, which makes sense given that e.g. the GE condition has a value of 7. The "condition to jump" description of the Cond field also shows only 3 options (made up of 2 bits), while there are now 5 natively supported conditions (at least according to the code).
  3. the ESP32-S2 ULP coprocessor instruction set documentation page still has an example of "how the EQ and GT conditions are implemented in the assembler" but at least from the code, it appears that all conditions are now natively supported by the JUMPS instruction.

…pr, jumps)

When providing negative immediate offset (step) values to the JUMPR and
JUMPS opcodes, the resulting instruction contained an incorrect offset.

This commit fixes that behaviour.

According to the technical reference manual (TRM)
[https://www.espressif.com/sites/default/files/documentation/esp32_technical_reference_manual_en.pdf]
the magnitude of the relative shift from the current position is determined
by bit 0-6 of the step field, and the direction is determined by bit 7,
with 0 meaning PC+step, and 1 meaning PC-step.

(For comparion, the ULP C macros in the ESP-IDF implement this as described
in the TRM. All step values passed to the relevant JUMP macros will result in the
instruction step field having bit 7 indicating the sign and bit 0-6 holding the
absolute value of the offset.)

This fix modifies the I_JUMP_REL{R,S} macros to set the step field correctly
for negative immediate values. Since symbols, which are resolved later during
the BFD relocation phase always evaluate to 0 at this stage (from EXPR_VALUE),
this change to the macro only affects the case of immediate values (as can be
seen from all previous test cases resulting in the same listing output as
before). The relocation code (in function esp32ulp_jumprelr_reloc) already did
this correctly for symbols, and thus remains unchanged.

Example of the issue:

For an offset of -2, the step field should have looked as follows:

  bit 0-6 = 2   # positive 2
  bit   7 = 1   # 1 means negative

However, the result was actually:

  bit 0-6 = 126  # negative 2 (two's compliment)
  bit   7 = 1    # 1 means negative
…umpr, jumps)

When providing negative immediate offset (step) values to the JUMPR and
JUMPS opcodes, the resulting instruction contained an incorrect offset.

This commit fixes that behaviour.

This is the same issue that affected the ESP32 code. See previous commit for
more technical detail on the issue.
@dmitry1945
Copy link
Contributor

Dear @wnienhaus,
thank you very much for such good work.

The changes now in test pipeline, and will be included into the masted soon as it is without any changes (this pull request should be closed automatically).

Thanks and Regards,
Dmitry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants