Skip to content

More fixes towards v1.0 #52

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 4, 2021
Merged

More fixes towards v1.0 #52

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1cb0db3
fix JUMP with immediate offsets (should get converted to words)
wnienhaus Sep 28, 2021
5e8ab92
fix dual-instruction jumps with immediate values
wnienhaus Sep 28, 2021
c9ff24d
fix ADC instruction when used with undocumented parameter
wnienhaus Sep 28, 2021
d90a0e8
improve line parsing
wnienhaus Sep 28, 2021
ff7db42
enable more skipped compat tests
wnienhaus Sep 29, 2021
6fa631a
fix comments syntax
wnienhaus Oct 3, 2021
b3ae442
improve line parsing, using regex to extract labels
wnienhaus Oct 3, 2021
78e9812
Add comment about dividing by 4
wnienhaus Oct 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion esp32_ulp/opcodes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,7 +619,7 @@ def i_jump(target, condition='--'):
raise ValueError("invalid flags condition")
if target.type == IMM or target.type == SYM:
_bx.dreg = 0
_bx.addr = get_abs(target)
_bx.addr = get_abs(target) if target.type == SYM else get_abs(target) >> 2 # bitwise version of "// 4"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get this.

If it is a symbol, it directly uses its value (absolute address) else it uses an immediate offset // 4?

Considering everything else in the opcode is the same, how does the cpu know what to do? like whether it is an offset or an absolute address?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In both cases what the machine instruction gets is an absolute address (not relative offset). The absolute address is either the address of a label, or an absolute address specified as immediate value.

What is different in the two cases is that the assembler instruction must take an address given in bytes, while the machine instruction needs to contain the address in words. Because in py-esp32-ulp we already track label addresses in words internally, we do not need to divide those symbol addresses by 4. But for immediate values we need to (see: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/ulp_instruction_set.html#note-about-addressing).

Does that make sense, or did I miss what you asked?

However... I did just discover one more case for the JUMP instruction, where we don't properly divide by 4. So this current logic is not 100% perfect. It appears to be that it's not only IMM values that must be divided by 4, but also symbol values that we internally label with the ABS type (as opposed to REL). Those ABS type symbols are defined by .set. So maybe the logic here should really be: Is what was given to us a relocatable thing (label) or not. If yes, use as is, if not divide by 4.

This logic is starting to look valid to me, the more I think about it. It should give the same result for the cases currently handled correctly, but also for the case of symbols defined with .set. It would requires a bit of a bigger change though, because of expression evaluation, which potentially uses multiple symbols that could have different types. So I will try and see how binutils-esp32ulp handles mixing symbols types, and likely a valid solution could be to consider the result of an expression to be a REL type if any symbols in the expression were of that type, otherwise consider the result to be an ABS type. And then use ABS and REL here rather than SYM and IMM to decide whether to divide by 4.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for explaining, please add a comment above this like:

# we track label addresses in 32bit words, but immediate values are in bytes and need to get divided by 4.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could also have ABS1 (byte addresses), ABS4 (32bit word addrs), etc. so we better know what we have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, my last part about the REL vs ABS, I think is not relevant anymore. I believe that "one more case" I talked about above, is actually a bug in binutils-esp32ulp

I have now tested quite a bit more and stepped through binutils-esp32ulp a bit and found the following: whether the value (that was defined with .set) gets divided by 4 depends on whether the symbol is marked as "global" or not. If marked as global, then the value will be taken as-is, when not marked as global, the value will be divided by 4. I don't see how this could be intended behaviour.

The reason I started to get skeptical is that binutils-esp32ulp behaves correctly for JUMPS and JUMPR cases, as per description in the documentation: https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-guides/ulp_instruction_set.html#note-about-addressing. The documentation shows an example, where values, which come from .set'ed symbols, should not be converted from bytes to words. So the fact that the JUMP instruction then tried to divide the value of the symbol by 4 (in the case I tried to fix) seems inconsistent.

I think the issues comes up in binutils-esp32ulp, depending on where the value is actually "filled-in" into the instruction. For non-global symbols, the value is already set during the assembly stage (as), while in the global case, the symbol value is only "filled in" during the linking stage (ld) - it's 0 in the instruction before that stage. The implementation of this logic in the assembler and linker differ.

So that means, I would not change anything in our implementation (other than adding the comment you suggested).

I might file a bug report to the binutils-esp32ulp project.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sounds strange, please file a bug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed a bug report now: espressif/binutils-esp32ulp#22

_bx.unused = 0
_bx.reg = 0
_bx.type = jump_type
Expand Down
6 changes: 6 additions & 0 deletions tests/compat/jumps.S
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@
entry:
nop

# simple jumps
jump entry
jump later
jump 0x120, EQ
jump -288, EQ

# jumps with labels
jumps entry, 42, lt
jumps entry, 42, lt
Expand Down