Page MenuHomePhabricator

[AVR] Don't adjust addresses by 2 for absolute values
ClosedPublic

Authored by aykevl on Feb 7 2020, 4:12 AM.

Details

Summary

Adjusting by 2 breaks DWARF output. With this fix, programs start to compile and produce valid DWARF output.


I'm not quite sure whether this is the correct fix. I feel like this patches over a bug somewhere else. Feel free to suggest a different way to do it.

Diff Detail

Event Timeline

aykevl created this revision.Feb 7 2020, 4:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 7 2020, 4:12 AM
aykevl edited the summary of this revision. (Show Details)Feb 7 2020, 4:13 AM
aykevl added a comment.Feb 7 2020, 1:23 PM

After some investigation, it appears that "temporary labels" can be constructed like this:

1:
    jmp 1b

I tried to make a test for this but it seems like adjustFixupValue is only called when actually generating an object file. Therefore, to test this we'd need to disassemble the code and check whether the raw bytes have the correct relocation addend. So I think a good test for this would need to wait until there is better disassembly support.

This comment was removed by dylanmckay.
dylanmckay requested changes to this revision.Feb 24 2020, 2:25 AM

Great work, thanks @aykevl.

I suspect it is possible to write a test for this using llvc -show-encoding

llc ~/projects/llvm/llvm-project/llvm/test/CodeGen/AVR/and.ll -o - --show-mc-encoding -march=avr -mcpu=atmega328p

Here's part of the output

andi    r18, 253                ; encoding: [0x2d,0x7f]
andi    r20, 155                ; encoding: [0x4b,0x79]
andi    r21, 88                 ; encoding: [0x58,0x75]
andi    r22, 76                 ; encoding: [0x6c,0x74]
andi    r23, 73                 ; encoding: [0x79,0x74]
andi    r24, 31                 ; encoding: [0x8f,0x71]
andi    r25, 242                ; encoding: [0x92,0x7f]
ret                             ; encoding: [0x08,0x95]

I believe that relocations will also be included in the output, as fixup values.

If that's not possible, then I agree that the tests will have to come in a pater patch.

This revision now requires changes to proceed.Feb 24 2020, 2:25 AM
aykevl updated this revision to Diff 246434.EditedFeb 25 2020, 6:46 AM
aykevl edited the summary of this revision. (Show Details)

I found a way to test at least the case where the value should not be shifted. If it is incorrectly shifted, the instruction becomes something other than a nop and thus will fail the test.
For the other case (when the value should be shifted), the AVR backend needs to be able to disassemble branch instructions. llc does not trigger the relevant code path.

This revision is now accepted and ready to land.Feb 25 2020, 9:00 PM
This revision was automatically updated to reflect the committed changes.