[X86] Fix a crash in FEntryInserter Pass.

[manojgupta]

Summary

FEntryInserter pass unconditionally derefs the first Instruction
in the first Basic Block. The pass crashes when the first
BasicBlock is empty. Fix the crash by not dereferencing the basic
Block iterator. This fixes an issue observed when building Linux kernel
4.4 with clang.

Fixes PR33971.

[davide]

Nice! What's the status of Linux 4.4 + clang?

lib/CodeGen/FEntryInserter.cpp

46 ↗

(On Diff #108595)

If the first BB is not empty, why you're not still passing `FirstMI.getDebugLoc()?

test/CodeGen/X86/fentry-insertion.ll

15 ↗

(On Diff #108595)

This comment is not needed.

29 ↗

(On Diff #108595)

Ditto.

[manojgupta]

Linux kernel is still WIP.

Added back the DebugLoc from first instruction. I am not sure though if reusing
debug location from an unrelated instruction is ok.

[manojgupta]

David, your comment didn't make it here. So adding it myself.

The call to fentry is unconditionally added to the beginning of function. Should the call use the same debug location of the previous instruction if present?

On Fri, Jul 28, 2017 at 10:33 AM, David Blaikie <[e-mail]> wrote:

Depends what you mean by "unrelated" - if the new instruction is created because of/only in the presence of the old instruction, yaeh, using its debugloc may be appropriate. (Though if the new instruction may be created in a different basic block - that gets trickier)

[manojgupta]

Removed getting DebugLoc from previous instruction based on Dave's comments.

[manojgupta]

From: David Blaikie <[e-mail]> Date: Fri, Jul 28, 2017 at 11:38 AM

• The call to fentry is unconditionally added to the beginning of function.

& the other instruction may not be in the entry block?

• Should the call use the same debug location of the previous instruction if present?

No - it'll probably get that location anyway, later on/at code generation time, but you shouldn't add it to the instruction itself if it doesn't have any reason to be related/at that location.

• Dave

[manojgupta]

Is it ok to land this?

[niravd]

LGTM.

llvm/trunk/lib/CodeGen/FEntryInserter.cpp

 
 bool FEntryInserter::runOnMachineFunction(MachineFunction &MF) {
   const std::string FEntryName =
       MF.getFunction()->getFnAttribute("fentry-call").getValueAsString();
   if (FEntryName != "true")
     return false;
 
   auto &FirstMBB = *MF.begin();
-  auto &FirstMI = *FirstMBB.begin();
-
   auto *TII = MF.getSubtarget().getInstrInfo();
-  BuildMI(FirstMBB, FirstMI, FirstMI.getDebugLoc(),
+  BuildMI(FirstMBB, FirstMBB.begin(), DebugLoc(),
           TII->get(TargetOpcode::FENTRY_CALL));
   return true;
 }
 
 char FEntryInserter::ID = 0;
 char &llvm::FEntryInserterID = FEntryInserter::ID;
 INITIALIZE_PASS(FEntryInserter, "fentry-insert", "Insert fentry calls", false,
                 false)

llvm/trunk/test/CodeGen/X86/fentry-insertion.ll

 ; RUN: llc %s -o - | FileCheck %s
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @test1() #0 {
 entry:
   ret void
 
 ; CHECK-LABEL: @test1
 ; CHECK: callq __fentry__
 ; CHECK-NOT: mcount
 ; CHECK: retq
 }
 
-attributes #0 = { "fentry-call"="true" }
+define void @test2() #1 {
+entry:
+  br label %bb1
+bb1:
+  call void @address_taken(i64 ptrtoint (i8* blockaddress(@test2, %bb1) to i64), i32 512)
+  ret void
 
+; CHECK-LABEL: @test2
+; CHECK: callq __fentry__
+; CHECK-NOT: mcount
+; CHECK: retq
+}
+
+declare void @address_taken(i64, i32) local_unnamed_addr
+attributes #0 = { "fentry-call"="true" }
+attributes #1 = { inlinehint minsize noredzone nounwind optsize sspstrong "fentry-call"="true" }