[PATCH] D73739: Exception support for basic block sections

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl marked 10 inline comments as done and 2 inline comments as done.
rahmanl added a comment.

In D73739#2264518 <https://reviews.llvm.org/D73739#2264518>, @MaskRay wrote:

> Taking a closer look at the test (haven't delved into the code yet): the PIC vs non-PIC difference of .gcc_exception_table looks strange.
> .gcc_except_table (please also test its section flags `"a"`) has an absolute relocation referencing `main.2`. This is not good. In all targets (except RISC-V -mno-relax), .gcc_except_table is a relocation-free section.
>
> I think you should just use PIC handling for -fno-PIC.

Thanks for the closer look. I included the section flags in the test.
Actually EHStreamer::emitTypeInfos already uses absolute and relative address (via calling getTTypeGlobalReference). This can be observed in the test:

; CHECK-NON-PIC-NEXT:   .long _ZTIi                     # TypeInfo 1
; CHECK-PIC-NEXT:     [[DOT:\.Ltmp[0-9]+]]:
; CHECK-PIC-NEXT:       .quad .L_ZTIi.DW.stub-[[DOT]]



================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.h:114
       SmallVectorImpl<CallSiteEntry> &CallSites,
+      Optional<SmallVector<CallSiteRange, 4>> &CallSiteRanges,
       const SmallVectorImpl<const LandingPadInfo *> &LandingPads,
----------------
modimo wrote:
> What's the reason for using Optional here? Does checking for size == 0 suffice?
Yes. It doesn't look great with Optional. I reverted the change.


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:298
+        ++MI;
+      MF.getSubtarget().getInstrInfo()->insertNoop(MBB, MI);
+      break;
----------------
modimo wrote:
> Using getNoop this should look something like:
> ```
>   MCInst Noop;
>   MF.getSubtarget().getInstrInfo()->getNoop(Noop);
>   BuildMI(MBB, MI, DL, MF.getSubtarget().getInstrInfo()->get(Noop.getOpcode()));
> ```
> I would also create a function to encapsulate this. Logically I think this fits here since you want to do this after `MF.assignBeginEndSections();` but before `updateBranches(MF, PreLayoutFallThroughs);` however the naming of `sortBasicBlocksAndUpdateBranches` is now not accurate. Personally I think a rename to `finalizeBasicBlocks` would work but I'll let @snehasish chime in here.
Great suggestion.
I added a new function avoidZeroOffsetLandingPad which can also be used in MachineFunctionSpliter.


================
Comment at: llvm/test/CodeGen/X86/gcc_except_table_bb_sections.ll:12
+; CHECK-PIC:        .cfi_personality 155, DW.ref.__gxx_personality_v0
+; CHECK-NON-PIC:    .cfi_lsda 3, .Lexception0
+; CHECK-PIC:        .cfi_lsda 27, .Lexception0
----------------
MaskRay wrote:
> personality/lsda encodings are difficult to remember. Worth a comment like `DW_EH_PE_pcrel | DW_EH_PE_indirect | DW_EH_PE_sdata4`
You're right, but the LSDA encoding is emitted by MCStreamer::emitCFILsda and I don't see any comment functionality there.


================
Comment at: llvm/test/CodeGen/X86/gcc_except_table_bb_sections.ll:15
+; CHECK-LABEL:    .Ltmp0:
+; CHECK-LABEL:    .Ltmp1:
+
----------------
MaskRay wrote:
> This is not sufficient. You need to check the next instruction to make sure things don't go off
Added instruction between the two labels. After Ltmp1 comes # %bb.1 which is not really helpful.


================
Comment at: llvm/test/CodeGen/X86/gcc_except_table_bb_sections.ll:19
+
+; CHECK-LABEL:    main.1:
+; CHECK:            .cfi_startproc
----------------
MaskRay wrote:
> If you use `main.1` `main.2` etc, you will waste a lot of space in `.strtab`. Better using a unary encoding or a temporary label (`.L` in ELF)
Your observation is correct. However, this patch does not touch that functionality (implemented in MachineBasicBlock::getSymbol()).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73739/new/

https://reviews.llvm.org/D73739