[PATCH] D73739: Exception support for basic block sections

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D73739#2301471 <https://reviews.llvm.org/D73739#2301471>, @rahmanl wrote:

> In D73739#2299885 <https://reviews.llvm.org/D73739#2299885>, @MaskRay wrote:
>
>> Let's take gcc_except_table_bb_sections.ll as the example:
>>
>> Call site sizes are incorrect
>> -----------------------------
>>
>>   .uleb128 .Lcst_end0-.Lcst_begin0  # Call Site Size
>>   
>>   .uleb128 .Lcst_end0-.Lcst_begin1  # Call Site Size
>>   
>>   .uleb128 .Lcst_end0-.Lcst_begin2  # Call Site Size
>>
>> I think the Call Site Sizes are incorrect. A pair of .Lcst_end0 and .Lcst_begin0 should exactly delimiter the call sites table, but the current implements uses `.Lcst_end0` for all 3 basic block sections and places  `.Lcst_end0` after the last Call Site Table.
>
> Yes. It's a bit confusing I agree. The label difference is used to locate the action table by the exception handling runtime. It also happens to be equal to the call site **table** size for no-basic-block-sections.
> With basic block sections, these offsets do not correspond to the call site table. We have one `cst_end` label and multiple `cst_begin` labels. However, correctness is guaranteed because each call site range explicitly includes the LSDA headers.
> One alternative is to change the name of these symbols similar to type table references (`cst_begin` to `atbaseref`  and   `cst_end` to `atbase`  where `at` represents Action Table) to make sure they're not incorrectly interpreted, but this will be applied to default too.

Thanks for the clarification. I think I understand it now. Conceptually the 3 call site tables overlap.`[[[ cst0 ] cst1 ] cst2 ] cst_end0: action_table`. This point may deserve a sentence in the description. I've left an inline comment about renaming `cst_end0`.

The trailing bytes of cst0 and cst1 are actually un-parseable parts. This does not matter in practice because these call site tables are all comprehensive. `__gxx_personality_v0` should be able to find a call site within cst0, if an exception is thrown in its range. __cxa_throw -> _Unwind_RaiseException -> find a CIE and the associated LSDA -> call `__gxx_personality_v0` -> find a call site in the call site table.

>> Action table
>> ------------
>>
>>> All these call-site tables will share their action and type tables.
>>
>> I think type tables can be shared but action tables cannot.
>>
>> In each call-site table, the action record offset of each call site record is relative to the action table the start of the action table. The action table follows the call-site table.
>>
>> "The action table follows the call-site table in the LSDA." If there are N call-site tables, there should be N corresponding action tables. See `libcxxabi/src/cxa_exceptions.cpp:scan_eh_tab` for how the table is parsed (`actionTableStart + (actionEntry - 1)`)
>
> Even with basic-block sections, there is still only one call-site table, but multiple call-site ranges. This works because each call-site range explicitly specifies references to action table and type table. So `actionTableStart` across all call-site ranges.
>
>> (Apologies that I only noticed this problem just now. I am learning exceptions while reviewing this patch...)
>
> No worries. Thanks for taking the time to understand and review.





================
Comment at: llvm/lib/CodeGen/AsmPrinter/EHStreamer.cpp:476
   Asm->OutStreamer->emitLabel(GCCETSym);
-  Asm->OutStreamer->emitLabel(Asm->getCurExceptionSym());
-
-  // Emit the LSDA header.
-  Asm->emitEncodingByte(dwarf::DW_EH_PE_omit, "@LPStart");
-  Asm->emitEncodingByte(TTypeEncoding, "@TType");
+  MCSymbol *CstEndLabel = Asm->createTempSymbol("cst_end");
 
----------------
if `CallSiteRanges.size() > 1`, consider renaming cst_end to something else, e.g. `action_table_start`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73739