Page MenuHomePhabricator

[AVR] Mark call-clobbered registers as clobbered in interrupt handlers
ClosedPublic

Authored by aykevl on Jan 3 2022, 11:40 AM.

Details

Summary

I have matched the RISCV backend, which only uses the interrupt save
list in getCalleeSavedRegs, _not_ in getCallPreservedMask. I don't know
the details of these two methods, but with it, the correct amount of
registers is saved and restored.

Without this patch, practically all interrupt handlers that call a
function will miscompile.

I have added a test to verify this behavior. I've also added a very
simple test to verify that more normal interrupt operations (in this
case, incrementing a global value) behave as expected.

Diff Detail

Event Timeline

aykevl created this revision.Jan 3 2022, 11:40 AM
aykevl requested review of this revision.Jan 3 2022, 11:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2022, 11:40 AM
aykevl planned changes to this revision.Jan 3 2022, 5:39 PM

Forgot the colon in CHECK-NEXT

aykevl updated this revision to Diff 397423.Jan 4 2022, 4:17 PM

Fixed missing semicolons in test

Generally speaking, I am OK with this patch. Just one suggestion, run llvm/utils/update_llc_test_checks.py against the test case llvm/test/CodeGen/AVR/interrupts.ll

llvm/test/CodeGen/AVR/interrupts.ll
1

Would it be better to add an option -verify-machineinstrs ?

115

Why this is a CHECK not a CHECK-NEXT ? Would it be better to show all auto generated assembly ?

145

Why this is a CHECK not a CHECK-NEXT ? Would it be better to show all auto generated assembly ?

aykevl updated this revision to Diff 397906.Jan 6 2022, 8:25 AM
  • replaced CHECK: with CHECK-NEXT:
aykevl added a comment.Jan 6 2022, 8:28 AM

Just one suggestion, run llvm/utils/update_llc_test_checks.py against the test case llvm/test/CodeGen/AVR/interrupts.ll

I think it is better to not do this for this test. With update_llc_test_checks.py, all the register names would be hidden. But the register names (r0, r1, etc) are an important part of this test.

llvm/test/CodeGen/AVR/interrupts.ll
1

Not sure. I've looked at other targets and most have it only on a small subset of tests.
If you want to enable the machine verifier, you can build LLVM with -DLLVM_ENABLE_EXPENSIVE_CHECKS=True or you can run llvm-lit manually, in my case:

../llvm-build.main/bin/llvm-lit -Dllc="llc -verify-machineinstrs" -sv llvm/test/CodeGen/AVR/

I usually use the above command to check for verification issues. This is how I found many issues that I fixed over several patches (see D107853#3224989).

aykevl updated this revision to Diff 397914.Jan 6 2022, 8:54 AM
  • rebased
aykevl updated this revision to Diff 397915.Jan 6 2022, 8:56 AM
  • rebase (try 2, previous one was the wrong patch)

That gets conformed with avr-gcc's behaviour on handling interrupt/signal service functions.

For the following C code

#include <avr/interrupt.h>
void foo(void);
SIGNAL(_VECTOR(1)) { foo(); }

avr-gcc generates the following asm, which is conformed with your patch.

        .file   "a.c"
__SP_H__ = 0x3e
__SP_L__ = 0x3d
__SREG__ = 0x3f
__tmp_reg__ = 0
__zero_reg__ = 1
        .text
.global __vector_1
        .type   __vector_1, @function
__vector_1:
        push r1
        push r0
        in r0,__SREG__
        push r0
        clr __zero_reg__
        push r18
        push r19
        push r20
        push r21
        push r22
        push r23
        push r24
        push r25
        push r26
        push r27
        push r30
        push r31
/* prologue: Signal */
/* frame size = 0 */
/* stack size = 15 */
.L__stack_usage = 15
        call foo
/* epilogue start */
        pop r31
        pop r30
        pop r27
        pop r26
        pop r25
        pop r24
        pop r23
        pop r22
        pop r21
        pop r20
        pop r19
        pop r18
        pop r0
        out __SREG__,r0
        pop r0
        pop r1
        reti
benshi001 accepted this revision.Jan 6 2022, 8:36 PM
This revision is now accepted and ready to land.Jan 6 2022, 8:36 PM