AArch64: Prefer FP-relative debug locations in HWASANified functions.

[pcc]

Summary

To help produce better diagnostics for stack use-after-return, we'd like
to be able to determine the addresses of each HWASANified function's local
variables given a small amount of information recorded on entry to the
function. Currently we require all HWASANified functions to use frame pointers
and record (PC, FP) on function entry. This works better than recording SP
because FP cannot change during the function, unlike SP which can change
e.g. due to dynamic alloca.

However, most variables currently end up using SP-relative locations in their
debug info. This prevents us from recomputing the address of most variables
because the distance between SP and FP isn't recorded in the debug info. To
address this, make the AArch64 backend prefer FP-relative debug locations
when producing debug info for HWASANified functions.

[pcc]

Oops, forgot to add a test. I'll do that in a bit.

[pcc]

• Add test

[eugenis]

What if we do it the other way around - store SP instead of FP? SP is unusable for locals when there are dynamic allocas; FP is unusable when there is dynamic stack realignment; not sure which case is more common, but both should be rare enough. This could help with the code size at least a little.

[pcc]

Don't we need frame pointers for fast stack traces anyway?

[eugenis]

In D63300#1543961, @eugenis wrote:

What if we do it the other way around - store SP instead of FP? SP is unusable for locals when there are dynamic allocas; FP is unusable when there is dynamic stack realignment; not sure which case is more common, but both should be rare enough. This could help with the code size at least a little.

Right, good point.

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

1538 ↗

(On Diff #204640)

should this be called ForDebugInfo, or something like that? It's not about signed immediate IMHO, it's about debug info having effectively infinite offset range.

[pcc]

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp

1538 ↗

(On Diff #204640)

I thought about giving it a name like that, but it seems like the restriction is specific to the operand on certain load and store instructions referred to as <simm>, or a 9-bit signed immediate. If we were producing an immediate for a different instruction, we would want this to be false.

That said, maybe it should be called something like ForLdSt, then.

[eugenis]

LGTM
I still think my suggestion for the name is best, but don't care about it too much.

llvm/trunk/lib/Target/AArch64/AArch64FrameLowering.h

public:
   void emitPrologue(MachineFunction &MF, MachineBasicBlock &MBB) const override;
   void emitEpilogue(MachineFunction &MF, MachineBasicBlock &MBB) const override;
 
   bool canUseAsPrologue(const MachineBasicBlock &MBB) const override;
 
   int getFrameIndexReference(const MachineFunction &MF, int FI,
                              unsigned &FrameReg) const override;
   int resolveFrameIndexReference(const MachineFunction &MF, int FI,
-                                 unsigned &FrameReg,
-                                 bool PreferFP = false) const;
+                                 unsigned &FrameReg, bool PreferFP,
+                                 bool ForSimm) const;
   bool spillCalleeSavedRegisters(MachineBasicBlock &MBB,
                                  MachineBasicBlock::iterator MI,
                                  const std::vector<CalleeSavedInfo> &CSI,
                                  const TargetRegisterInfo *TRI) const override;
 
   bool restoreCalleeSavedRegisters(MachineBasicBlock &MBB,
                                   MachineBasicBlock::iterator MI,
                                   std::vector<CalleeSavedInfo> &CSI,

llvm/trunk/lib/Target/AArch64/AArch64FrameLowering.cpp

 
 /// getFrameIndexReference - Provide a base+offset reference to an FI slot for
 /// debug info.  It's the same as what we use for resolving the code-gen
 /// references for now.  FIXME: This can go wrong when references are
 /// SP-relative and simple call frames aren't used.
 int AArch64FrameLowering::getFrameIndexReference(const MachineFunction &MF,
                                                  int FI,
                                                  unsigned &FrameReg) const {
-  return resolveFrameIndexReference(MF, FI, FrameReg);
+  return resolveFrameIndexReference(
+      MF, FI, FrameReg,
+      /*PreferFP=*/
+      MF.getFunction().hasFnAttribute(Attribute::SanitizeHWAddress),
+      /*ForSimm=*/false);
 }
 
 int AArch64FrameLowering::getNonLocalFrameIndexReference(
   const MachineFunction &MF, int FI) const {
   return getSEHFrameIndexOffset(MF, FI);
 }
 
 static int getFPOffset(const MachineFunction &MF, int FI) {
int AArch64FrameLowering::getSEHFrameIndexOffset(const MachineFunction &MF,
   const auto *RegInfo = static_cast<const AArch64RegisterInfo *>(
       MF.getSubtarget().getRegisterInfo());
   return RegInfo->getLocalAddressRegister(MF) == AArch64::FP ?
          getFPOffset(MF, FI) : getStackOffset(MF, FI);
 }
 
 int AArch64FrameLowering::resolveFrameIndexReference(const MachineFunction &MF,
                                                      int FI, unsigned &FrameReg,
-                                                     bool PreferFP) const {
+                                                     bool PreferFP,
+                                                     bool ForSimm) const {
   const auto &MFI = MF.getFrameInfo();
   const auto *RegInfo = static_cast<const AArch64RegisterInfo *>(
       MF.getSubtarget().getRegisterInfo());
   const auto *AFI = MF.getInfo<AArch64FunctionInfo>();
   const auto &Subtarget = MF.getSubtarget<AArch64Subtarget>();
   int FPOffset = getFPOffset(MF, FI);
   int Offset = getStackOffset(MF, FI);
   bool isFixed = MFI.isFixedObjectIndex(FI);
if (isFixed) {
       UseFP = hasFP(MF);
     } else if (isCSR && RegInfo->needsStackRealignment(MF)) {
       // References to the CSR area must use FP if we're re-aligning the stack
       // since the dynamically-sized alignment padding is between the SP/BP and
       // the CSR area.
       assert(hasFP(MF) && "Re-aligned stack must have frame pointer");
       UseFP = true;
     } else if (hasFP(MF) && !RegInfo->needsStackRealignment(MF)) {
-      // If the FPOffset is negative, we have to keep in mind that the
-      // available offset range for negative offsets is smaller than for
-      // positive ones. If an offset is
-      // available via the FP and the SP, use whichever is closest.
-      bool FPOffsetFits = FPOffset >= -256;
+      // If the FPOffset is negative and we're producing a signed immediate, we
+      // have to keep in mind that the available offset range for negative
+      // offsets is smaller than for positive ones. If an offset is available
+      // via the FP and the SP, use whichever is closest.
+      bool FPOffsetFits = !ForSimm || FPOffset >= -256;
       PreferFP |= Offset > -FPOffset;
 
       if (MFI.hasVarSizedObjects()) {
         // If we have variable sized objects, we can use either FP or BP, as the
         // SP offset is unknown. We can use the base pointer if we have one and
         // FP is not preferred. If not, we're stuck with using FP.
         bool CanUseBP = RegInfo->hasBasePointer(MF);
         if (FPOffsetFits && CanUseBP) // Both are ok. Pick the best.

llvm/trunk/lib/Target/AArch64/AArch64RegisterInfo.cpp

void AArch64RegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II,
   int FrameIndex = MI.getOperand(FIOperandNum).getIndex();
   unsigned FrameReg;
   int Offset;
 
   // Special handling of dbg_value, stackmap and patchpoint instructions.
   if (MI.isDebugValue() || MI.getOpcode() == TargetOpcode::STACKMAP ||
       MI.getOpcode() == TargetOpcode::PATCHPOINT) {
     Offset = TFI->resolveFrameIndexReference(MF, FrameIndex, FrameReg,
-                                             /*PreferFP=*/true);
+                                             /*PreferFP=*/true,
+                                             /*ForSimm=*/false);
     Offset += MI.getOperand(FIOperandNum + 1).getImm();
     MI.getOperand(FIOperandNum).ChangeToRegister(FrameReg, false /*isDef*/);
     MI.getOperand(FIOperandNum + 1).ChangeToImmediate(Offset);
     return;
   }
 
   if (MI.getOpcode() == TargetOpcode::LOCAL_ESCAPE) {
     MachineOperand &FI = MI.getOperand(FIOperandNum);
     Offset = TFI->getNonLocalFrameIndexReference(MF, FrameIndex);
     FI.ChangeToImmediate(Offset);
     return;
   }
 
   // Modify MI as necessary to handle as much of 'Offset' as possible
-  Offset = TFI->getFrameIndexReference(MF, FrameIndex, FrameReg);
+  Offset = TFI->resolveFrameIndexReference(
+      MF, FrameIndex, FrameReg, /*PreferFP=*/false, /*ForSimm=*/true);
 
   if (rewriteAArch64FrameIndex(MI, FIOperandNum, FrameReg, Offset, TII))
     return;
 
   assert((!RS || !RS->isScavengingFrameIndex(FrameIndex)) &&
          "Emergency spill slot is out of reach");
 
   // If we get here, the immediate doesn't fit into the instruction.  We folded

llvm/trunk/test/CodeGen/AArch64/hwasan-prefer-fp.ll

+; RUN: llc -o - %s -filetype=obj -frame-pointer=all | llvm-dwarfdump - | FileCheck %s
+
+target triple="aarch64--"
+
+define void @f() sanitize_hwaddress !dbg !6 {
+entry:
+  %x = call i8* @g(i32 0)
+  %a = alloca [128 x i8]
+  %b = alloca [128 x i8]
+  ; CHECK: DW_AT_location (DW_OP_fbreg
+  call void @llvm.dbg.declare(metadata [128 x i8]* %a, metadata !12, metadata !DIExpression()), !dbg !14
+  ; CHECK: DW_AT_location (DW_OP_fbreg
+  call void @llvm.dbg.declare(metadata [128 x i8]* %b, metadata !13, metadata !DIExpression()), !dbg !14
+  ret void, !dbg !15
+}
+
+declare i8* @g(i32)
+
+declare void @llvm.dbg.declare(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "x.c", directory: "/")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{!"clang"}
+!6 = distinct !DISubprogram(name: "f", scope: !1, file: !1, line: 1, type: !7, isLocal: false, isDefinition: true, scopeLine: 1, flags:
+DIFlagPrototyped, isOptimized: false, unit: !0, retainedNodes: !2)
+!7 = !DISubroutineType(types: !8)
+!8 = !{null, !9}
+!9 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !10, size: 64)
+!10 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !11)
+!11 = !DIBasicType(name: "char", size: 8, encoding: DW_ATE_signed_char)
+!12 = !DILocalVariable(name: "a", scope: !6, file: !1, line: 1, type: !9)
+!13 = !DILocalVariable(name: "b", scope: !6, file: !1, line: 1, type: !9)
+!14 = !DILocation(line: 1, column: 29, scope: !6)
+!15 = !DILocation(line: 1, column: 37, scope: !6)