[PatchableFunction] Allow empty entry MachineBasicBlock

[nickdesaulniers]

Does https://godbolt.org/z/MxVVjE help?

[nickdesaulniers]

(sorry, crash is different w/ and w/o --target=aarch64-linux-gnu)

[nickdesaulniers]

Also, I just used llvm-reduce for the first time to get:

; ModuleID = 'quarantine.ll'
source_filename = "quarantine.i"
target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
target triple = "aarch64-unknown-linux-gnu"
; Function Attrs: nounwind
define dso_local i32 @a() #0 {
entry:
  ret void
}

attributes #0 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "frame-pointer"="non-leaf" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "patchable-function-entry"="0" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" "unsafe-fp-math"="false" "use-soft-float"="false" }

!llvm.module.flags = !{!0}

!0 = !{i32 1, !"wchar_size", i32 4}

[MaskRay]

Add MIR test

[MaskRay]

Improve test

[merge_guards_bot]

Unit tests: fail. 62146 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: fail. 62147 tests passed, 5 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: fail. 62146 tests passed, 6 failed and 811 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp
failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_timedmutex_requirements/thread_timedmutex_recursive/try_lock_until.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[nickdesaulniers]

Thanks, with this patch applied, I was able to build an allyesconfig aarch64 linux-next kernel, and the llvm test suite passes. LGTM

[MaskRay]

Simplify test

[MaskRay]

In D73301#1839138, @nickdesaulniers wrote:

Thanks, with this patch applied, I was able to build an allyesconfig aarch64 linux-next kernel, and the llvm test suite passes. LGTM

What is the linux-next kernel?

I tested with (it needed a -DLLVM_ENABLE_ASSERTIONS=on build to trigger an assertion failure in ilist_iterator.h):

make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- HOSTGCC=gcc CC=~/llvm/ReleaseAssert/bin/clang LD=~/llvm/ReleaseAssert/bin/ld.lld O=/tmp/arm64 allmodconfig all -j 30

[MaskRay]

@hans OK to cherry pick this commit? We would also need d232c215669cb57f5eb4ead40a4a336220dbc429 ([AsmPrinter] Don't emit __patchable_function_entries entry if "patchable-function-entry"="0"; a GCC compatibility issue I did not notice when I implemented -fpatchable-function-entry=N,0).

If users want to see a complete -fpatchable-function-entry=N,M, instead of just -fpatchable-function-entry=N,0 , in clang 10.0.0, we will need D73070, D73071, and D73072 as well (N,M where M>0 is currently not used, though someone told me that Linux arch/arm64 may want to do that in the future. If that is the case, supporting a complete N,M, not just N,0 can simplify Linux kernel side configure test.)

[merge_guards_bot]

Unit tests: fail. 62175 tests passed, 5 failed and 815 were skipped.

failed: libc++.std/language_support/cmp/cmp_partialord/partialord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongeq/cmp.strongeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_strongord/strongord.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakeq/cmp.weakeq.pass.cpp
failed: libc++.std/language_support/cmp/cmp_weakord/weakord.pass.cpp

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[hans]

In D73301#1839163, @MaskRay wrote:

@hans OK to cherry pick this commit? We would also need d232c215669cb57f5eb4ead40a4a336220dbc429 ([AsmPrinter] Don't emit __patchable_function_entries entry if "patchable-function-entry"="0"; a GCC compatibility issue I did not notice when I implemented -fpatchable-function-entry=N,0).

Yes, go ahead.

If users want to see a complete -fpatchable-function-entry=N,M, instead of just -fpatchable-function-entry=N,0 , in clang 10.0.0, we will need D73070, D73071, and D73072 as well (N,M where M>0 is currently not used, though someone told me that Linux arch/arm64 may want to do that in the future. If that is the case, supporting a complete N,M, not just N,0 can simplify Linux kernel side configure test.)

I think you're the best judge of whether we should merge these. I'm okay either way.

llvm/lib/CodeGen/PatchableFunction.cpp

static bool doesNotGeneratecode(const MachineInstr &MI) {
   case TargetOpcode::DBG_LABEL:
     return true;
   }
 }
 
 bool PatchableFunction::runOnMachineFunction(MachineFunction &MF) {
   if (MF.getFunction().hasFnAttribute("patchable-function-entry")) {
     MachineBasicBlock &FirstMBB = *MF.begin();
-    MachineInstr &FirstMI = *FirstMBB.begin();
     const TargetInstrInfo *TII = MF.getSubtarget().getInstrInfo();
+    if (FirstMBB.empty()) {
+      BuildMI(&FirstMBB, DebugLoc(),
+              TII->get(TargetOpcode::PATCHABLE_FUNCTION_ENTER));
+    } else {
+      MachineInstr &FirstMI = *FirstMBB.begin();
       BuildMI(FirstMBB, FirstMI, FirstMI.getDebugLoc(),
               TII->get(TargetOpcode::PATCHABLE_FUNCTION_ENTER));
+    }
     return true;
   }
 
   if (!MF.getFunction().hasFnAttribute("patchable-function"))
     return false;
 
 #ifndef NDEBUG
   Attribute PatchAttr = MF.getFunction().getFnAttribute("patchable-function");

llvm/test/CodeGen/AArch64/patchable-function-entry-empty.mir

+# RUN: llc -mtriple=aarch64 -run-pass=patchable-function %s -o - | FileCheck %s
+
+# CHECK:    name: empty
+# CHECK:    bb.0.entry
+# CHECK:      PATCHABLE_FUNCTION_ENTER debug-location !DILocation(line: 1,
+# CHECK-NEXT: RET undef $lr, debug-location !DILocation(line: 1,
+
+## Empty entry MBB, no debug location.
+# CHECK:    name: empty_entry
+# CHECK:    bb.0.entry
+# CHECK:      PATCHABLE_FUNCTION_ENTER{{$}}
+# CHECK:    bb.1.here
+
+--- |
+  define void @empty() #0 !dbg !7 {
+  entry:
+    ret void, !dbg !10
+  }
+
+  define void @empty_entry() #0 !dbg !11 {
+  entry:
+    br label %here
+  here:
+    ret void, !dbg !12
+  }
+
+  attributes #0 = { "patchable-function-entry"="1" }
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "a.c", directory: "/tmp")
+  !2 = !{}
+  !3 = !{i32 7, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{!"clang version 11.0.0 "}
+  !7 = distinct !DISubprogram(name: "empty", scope: !1, file: !1, line: 1, type: !8, scopeLine: 1, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{null}
+  !10 = !DILocation(line: 1, column: 61, scope: !7)
+  !11 = distinct !DISubprogram(name: "empty_entry", scope: !1, file: !1, line: 2, type: !8, scopeLine: 2, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+  !12 = !DILocation(line: 2, column: 61, scope: !11)
+
+...
+---
+name:            empty
+alignment:       4
+tracksRegLiveness: true
+body:             |
+  bb.0.entry:
+    liveins: $lr
+    RET undef $lr, debug-location !10
+
+...
+---
+name:            empty_entry
+alignment:       4
+tracksRegLiveness: true
+body:             |
+  bb.0.entry:
+    liveins: $lr
+  bb.1.here:
+    liveins: $lr
+    RET undef $lr, debug-location !12
+
+...