[zero-call-used-regs] Mark only non-debug instruction's register as used

[xgupta]

Summary

zero-call-used-regs pass generate an xor instruction to help mitigate
return-oriented programming exploits via zeroing out used registers. But
in this below test case with -g option there is dbg.value instruction
associating the register with the debug-info description of the formal
parameter d, which makes the register appear used, therefore it zero the
register edi in -g case and makes binary different from without -g option.

The pass should be looking only at the non-debug uses.

$ cat test.c
char a[];
int b;
attribute((zero_call_used_regs("used"))) char c(int d) {

*a = ({
  int e = d;
  b;
});

}

This fixes https://github.com/llvm/llvm-project/issues/57962.

[dblaikie]

Could you add a test case?

llvm/lib/CodeGen/PrologEpilogInserter.cpp

1241–1243

probably makes sense to put this outside the MI.operands() loop, so the loop isn't even run for debug instnructions

[xgupta]

In D138757#3952422, @dblaikie wrote:

Could you add a test case?

I had tried but assertions are not generating with update_llc_test_checks.py when -g flag specified with -S -emit-llvm. Should I manually write CHECK lines in test case?

[xgupta]

address comments

[dblaikie]

In D138757#3952450, @xgupta wrote:

In D138757#3952422, @dblaikie wrote:

Could you add a test case?

I had tried but assertions are not generating with update_llc_test_checks.py when -g flag specified with -S -emit-llvm. Should I manually write CHECK lines in test case?

Can't say I'm especially familiar with update_llc_test_checks.py and where it could/should be used (I guess by the name, it's for updating existing tests - so maybe this needs a new test written?) But, yes, either way, may require writing a test with hand-crafted CHECK lines.

llvm/lib/CodeGen/PrologEpilogInserter.cpp

1232

Need some curly braces on this loop now? (& the indentation of the isDebugStr check looks like it's off-by-one)

[xgupta]

address comments

[xgupta]

Can't say I'm especially familiar with update_llc_test_checks.py and where it could/should be used (I guess by the name, it's for updating existing tests - so maybe this needs a new test written?) But, yes, either way, may require writing a test with hand-crafted CHECK lines.

OK, Does it requires mir, assembly or llvm ir testcase because there is no change at ir level wrt -g or without -g, see https://godbolt.org/z/ndz36Kn3P.

llvm/lib/CodeGen/PrologEpilogInserter.cpp

1232

Yeah, missed that.

[dblaikie]

In D138757#3955993, @xgupta wrote:

Can't say I'm especially familiar with update_llc_test_checks.py and where it could/should be used (I guess by the name, it's for updating existing tests - so maybe this needs a new test written?) But, yes, either way, may require writing a test with hand-crafted CHECK lines.

OK, Does it requires mir, assembly or llvm ir testcase because there is no change at ir level wrt -g or without -g, see https://godbolt.org/z/ndz36Kn3P.

I'd probably do it in IR->asm, but other folks might be more familiar with how to do this as a narrower MIR test or the like.

[nickdesaulniers]

Thanks for the patch! Sorry for the delay reviewing; a few of us where off last week for the Thanksgiving holiday.

Change LGTM; but this must have a test added.

In D138757#3952450, @xgupta wrote:

In D138757#3952422, @dblaikie wrote:

Could you add a test case?

I had tried but assertions are not generating with update_llc_test_checks.py when -g flag specified with -S -emit-llvm. Should I manually write CHECK lines in test case?

Did you add a ; RUN: line to the beginning of your test BEFORE running update_llc_checks.py? Forgetting to do so is a common way for those scripts not to make changes. (I'll also recommend using llvm-reduce to minimize the .ll reproducer).

[nickdesaulniers]

In D138757#3957674, @dblaikie wrote:

In D138757#3955993, @xgupta wrote:

Can't say I'm especially familiar with update_llc_test_checks.py and where it could/should be used (I guess by the name, it's for updating existing tests - so maybe this needs a new test written?) But, yes, either way, may require writing a test with hand-crafted CHECK lines.

OK, Does it requires mir, assembly or llvm ir testcase because there is no change at ir level wrt -g or without -g, see https://godbolt.org/z/ndz36Kn3P.

Not true: https://godbolt.org/z/6516an1rh
Without -g, there is no calls to the @llvm.dbg.value intrinsic or !dbg metadata nodes.

I'd probably do it in IR->asm, but other folks might be more familiar with how to do this as a narrower MIR test or the like.

Ah, they joys of writing MIR tests...a MIR test is appropriate, since this is only one pass we need to test; PEI.

To help generate a test, I recommend:

1. dump IR
2. llvm-reduce IR
3. llc -O2 -stop-before=prolog-epilog-insertion -o new_mir_test.mir
4. add the ; RUN: line to new_mir_test.mir: ; RUN: llc -run-pass=prolog-epilog-insertion ...

That way you don't have to write MIR by hand, and we get a concise and quick test.

Or you can simply add another IR test case to the existing test for this feature.
llvm/test/CodeGen/X86/zero-call-used-regs.ll
or
llvm/test/CodeGen/AArch64/zero-call-used-regs.ll
would be appropriate.

I don't care which you do. The IR->asm test is probably easier; writing MIR tests builds character (and they run faster, since we're running less of the compilation pipeline).

[nickdesaulniers]

In D138757#3957857, @nickdesaulniers wrote:

3. llc -O2 -stop-before=prolog-epilog-insertion -o new_mir_test.mir
4. add the ; RUN: line to new_mir_test.mir: ; RUN: llc -run-pass=prolog-epilog-insertion ...

Ah, looks like PEI is called prologepilog not prolog-epilog-insertion.

You can generally find the pass identifier as DEBUG_TYPE near the top of the file. These don't always correspond 1:1 with the pass name (for complicated passes in multiple files, they usually have individual DEBUG_TYPEs), but a common pattern in LLVM is that they do.

[xgupta]

add test case

[xgupta]

Hi,

I am not able to complete 2nd and 4th part. Any idea what may be wrong.

Thanks,
shivam

2. llvm-reduce IR

xgupta@archlinux ~/compiler/llvm-project/build/bin (arcpatch-D138757) $ ./llvm-reduce --test=test.sh pr57962.ll                

Input isn't interesting! Verify interesting-ness test

xgupta@archlinux ~/compiler/llvm-project/build/bin (arcpatch-D138757) $ cat test.sh 
#!/usr/bin/env bash
$HOME/compiler/llvm-project/build/bin/llc  $1 |& grep PrologEpilogInserter

4. add the ; RUN: line to new_mir_test.mir: ; RUN: llc -run-pass=prolog-epilog-insertion ...

xgupta@archlinux ~/compiler/llvm-project (arcpatch-D138757?) $ ./llvm/utils/update_mir_test_checks.py llvm/test/CodeGen/X86/pr57962.mir 
error: YAML:99:1: unknown key 'debugInstrRef'
debugInstrRef:   true
^~~~~~~~~~~~~
WARNING: Error processing file: llvm/test/CodeGen/X86/pr57962.mir
Traceback (most recent call last):
  File "/home/xgupta/compiler/llvm-project/./llvm/utils/update_mir_test_checks.py", line 456, in <module>
    main()
  File "/home/xgupta/compiler/llvm-project/./llvm/utils/update_mir_test_checks.py", line 449, in main
    update_test_file(ti.args, ti.path, ti.test_autogenerated_note)
  File "/home/xgupta/compiler/llvm-project/./llvm/utils/update_mir_test_checks.py", line 343, in update_test_file
    raw_tool_output = args.llc_binary(llc_args, test)
  File "/home/xgupta/compiler/llvm-project/./llvm/utils/update_mir_test_checks.py", line 74, in __call__
    stdout = subprocess.check_output('{} {}'.format(self.bin, args),
  File "/usr/lib/python3.10/subprocess.py", line 421, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/usr/lib/python3.10/subprocess.py", line 526, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command 'llc -run-pass=prologepilog -o - -x mir' returned non-zero exit status 1.

[jmorse]

In D138757#4071031, @xgupta wrote:

I am not able to complete 2nd and 4th part. Any idea what may be wrong.

[...]

error: YAML:99:1: unknown key 'debugInstrRef'
debugInstrRef: true
^~~~~~~~~~~~~

[...]

subprocess.CalledProcessError: Command 'llc -run-pass=prologepilog -o - -x mir' returned non-zero exit status 1.

Ah, this particular fault is because I recently added a new element to MIR files. It would appear that you're using an older version of llc to read the output of a newer llc -- you should be able to just rebuild llc and it will work, or in the worst case, rebase onto a commit more recent than rG9f8544713ad8e57.

Alternately, you can just delete the lines "debugInstrRef: true" from the tests safely, as you're not testing anything to do with that feature

[xgupta]

update test case for CHECK lines.

[xgupta]

use --llc-binary build/bin/llc to correcting update test case

[xgupta]

Ah, this particular fault is because I recently added a new element to MIR files. It would appear that you're using an older version of llc to read the output of a newer llc -- you should be able to just rebuild llc and it will work, or in the worst case, rebase onto a commit more recent than rG9f8544713ad8e57.

Thanks, I was using system-installed llc to update check lines, using --llc-binary fixes the issue.

[nickdesaulniers]

llvm/test/CodeGen/X86/pr57962.mir

1 ↗

(On Diff #491467)

Do you mind renaming this test from pr57962.mir to zero-call-used-regs-debug-info.mir.

3 ↗

(On Diff #491467)

Please add a comment along the lines of:

# Test that the presence of debug instructions doesn't trigger arbitrary registers that are unused to be zeroed out.

146–198 ↗

(On Diff #491467)

all of this will go away if you remove @main and rerun utils/update_mir_test_checks.py.

24–28 ↗

(On Diff #491462)

you can delete @main from this test, it's unnecessary.

Probably the metadata it references will need to be cleaned up, too.

[MatzeB]

llvm/lib/CodeGen/PrologEpilogInserter.cpp

1233–1235

Can you just use MO.readsReg() instead of MO.isUse() below (that will also skip other cases like undef that aren't really reading the register).

1242

Haven't followed this patch series. But skipping implicit operands seems odd at a first glance. They can read and write registers perfectly fine...

[MatzeB]

llvm/lib/CodeGen/PrologEpilogInserter.cpp

1233–1235

Hmm I thought MO.readsReg() would check for MO.isDebug() but apparently it does not. So this check here is fine, sorry for the confusion.

[xgupta]

address comments

[xgupta]

llvm/lib/CodeGen/PrologEpilogInserter.cpp

1242

not sure, maybe @nickdesaulniers or @void can comment on skipping implicit operands.

[nickdesaulniers]

Thanks for the work on this patch! MIR tests aren't the most fun to write/generate, but doing so builds muscle!

[xgupta]

In D138757#4078593, @nickdesaulniers wrote:

Thanks for the work on this patch! MIR tests aren't the most fun to write/generate, but doing so builds muscle!

Thank you for reviewing and helping me to submit it.

llvm/lib/CodeGen/PrologEpilogInserter.cpp

void PEI::insertZeroCallUsedRegs(MachineFunction &MF) {
 
   const TargetRegisterInfo &TRI = *MF.getSubtarget().getRegisterInfo();
   const BitVector AllocatableSet(TRI.getAllocatableSet(MF));
 
   // Mark all used registers.
   BitVector UsedRegs(TRI.getNumRegs());
   if (OnlyUsed)
     for (const MachineBasicBlock &MBB : MF)
       for (const MachineInstr &MI : MBB)
+         // skip debug instructions
+         if (MI.isDebugInstr())
+           continue;
+
         for (const MachineOperand &MO : MI.operands()) {
           if (!MO.isReg())
             continue;
 
           MCRegister Reg = MO.getReg();
           if (AllocatableSet[Reg] && !MO.isImplicit() &&
               (MO.isDef() || MO.isUse()))
             UsedRegs.set(Reg);
         }
 
   // Get a list of registers that are used.
   BitVector LiveIns(TRI.getNumRegs());
   for (const MachineBasicBlock::RegisterMaskPair &LI : MF.front().liveins())
     LiveIns.set(LI.PhysReg);