diff mbox series

Fix PR target/81361

Message ID 2507479.MzcqCaxoQO@polaris
State New
Headers show
Series Fix PR target/81361 | expand

Commit Message

Eric Botcazou Sept. 18, 2017, 7:50 a.m. UTC 
Hi,

exception handling is currently broken in all languages for Darwin at -O2 on 
the mainline because of what appears to be a bug in either the assembler or 
the system unwinder.  The problem occurs when the compiler decides to split a 
function into hot & cold parts and the cold part is active wrt EH; in this 
case, the compiler generates a FDE for each part (the Darwin port doesn't use 
the CFI assembler directives) and the second FDE looks like:

.set L$set$24,LEFDE3-LASFDE3
	.long L$set$24	# FDE Length
LASFDE3:
	.long	LASFDE3-EH_frame1	# FDE CIE offset
	.quad	LCOLDB1-.	# FDE initial location
	.set L$set$25,LCOLDE1-LCOLDB1
	.quad L$set$25	# FDE address range
	.byte	0x8	# uleb128 0x8; Augmentation size
	.quad	LLSDAC5-.	# Language Specific Data Area
	.byte	0x1	# DW_CFA_set_loc
	.quad	LCFI1-.
	.byte	0xe	# DW_CFA_def_cfa_offset
	.byte	0x10	# uleb128 0x10
	.byte	0x83	# DW_CFA_offset, column 0x3
	.byte	0x2	# uleb128 0x2

Note the DW_CFA_set_loc operation: it's the only case where the compiler emits 
it (DW_CFA_advance_loc4 is usually emitted) and is the source of the problem, 
since it appears that the PC-relative relocation is not applied to the operand 
of the DW_CFA_set_loc (unlike to the 2 other cases in the FDE).

This DW_CFA_set_loc instruction is emitted by add_cfis_to_fde for the second 
FDE generated for the cold part of a function but doesn't seem necessary any 
more, since there is a label (LCOLDB1) to be used now (this can also be seen 
on Linux with the -fno-dwarf2-cfi-asm option).

Bootstrapped/regtested on x86-64/Linux by me and various versions of Darwin by 
Iain, Dominique and myself.  OK for the mainline?


2017-09-18  Eric Botcazou  <ebotcazou@adacore.com>

	PR target/81361
	* dwarf2cfi.c (add_cfis_to_fde): Do not generate DW_CFA_set_loc
	after switching to a new text section.

Comments

Jakub Jelinek Sept. 18, 2017, 8:30 a.m. UTC | #1 
On Mon, Sep 18, 2017 at 09:50:45AM +0200, Eric Botcazou wrote:
> .set L$set$24,LEFDE3-LASFDE3
> 	.long L$set$24	# FDE Length
> LASFDE3:
> 	.long	LASFDE3-EH_frame1	# FDE CIE offset
> 	.quad	LCOLDB1-.	# FDE initial location
> 	.set L$set$25,LCOLDE1-LCOLDB1
> 	.quad L$set$25	# FDE address range
> 	.byte	0x8	# uleb128 0x8; Augmentation size
> 	.quad	LLSDAC5-.	# Language Specific Data Area
> 	.byte	0x1	# DW_CFA_set_loc
> 	.quad	LCFI1-.
> 	.byte	0xe	# DW_CFA_def_cfa_offset
> 	.byte	0x10	# uleb128 0x10
> 	.byte	0x83	# DW_CFA_offset, column 0x3
> 	.byte	0x2	# uleb128 0x2
> 
> Note the DW_CFA_set_loc operation: it's the only case where the compiler emits 
> it (DW_CFA_advance_loc4 is usually emitted) and is the source of the problem, 
> since it appears that the PC-relative relocation is not applied to the operand 
> of the DW_CFA_set_loc (unlike to the 2 other cases in the FDE).

That sounds like a Darwin bug in handling of DW_CFA_set_loc in .eh_frame
section, the encoding/size of the DW_CFA_set_loc operand is an encoded
pointer, always absolute address in .debug_frame section, and whatever the
CIE augmentation says should be used otherwise, i.e. the same as e.g. FDE
initial location pointer.  As that is LCOLDB1-. in the same FDE, it means
LCFI1-. is right.

That said, there is indeed no reason to emit DW_CFA_set_loc when we have a
label, so your patch is ok for trunk.  That doesn't mean Darwin shouldn't be
fixed.  libgcc unwind-dw2.c for DW_CFA_set_loc uses read_encoded_value
and so I believe should DTRT.

> This DW_CFA_set_loc instruction is emitted by add_cfis_to_fde for the second 
> FDE generated for the cold part of a function but doesn't seem necessary any 
> more, since there is a label (LCOLDB1) to be used now (this can also be seen 
> on Linux with the -fno-dwarf2-cfi-asm option).
> 
> Bootstrapped/regtested on x86-64/Linux by me and various versions of Darwin by 
> Iain, Dominique and myself.  OK for the mainline?
> 
> 
> 2017-09-18  Eric Botcazou  <ebotcazou@adacore.com>
> 
> 	PR target/81361
> 	* dwarf2cfi.c (add_cfis_to_fde): Do not generate DW_CFA_set_loc
> 	after switching to a new text section.

	Jakub
diff mbox series

Patch

Index: dwarf2cfi.c
===================================================================
--- dwarf2cfi.c	(revision 252749)
+++ dwarf2cfi.c	(working copy)
@@ -2209,20 +2209,13 @@  add_cfis_to_fde (void)
 {
   dw_fde_ref fde = cfun->fde;
   rtx_insn *insn, *next;
-  /* We always start with a function_begin label.  */
-  bool first = false;
 
   for (insn = get_insns (); insn; insn = next)
     {
       next = NEXT_INSN (insn);
 
       if (NOTE_P (insn) && NOTE_KIND (insn) == NOTE_INSN_SWITCH_TEXT_SECTIONS)
-	{
-	  fde->dw_fde_switch_cfi_index = vec_safe_length (fde->dw_fde_cfi);
-	  /* Don't attempt to advance_loc4 between labels
-	     in different sections.  */
-	  first = true;
-	}
+	fde->dw_fde_switch_cfi_index = vec_safe_length (fde->dw_fde_cfi);
 
       if (NOTE_P (insn) && NOTE_KIND (insn) == NOTE_INSN_CFI)
 	{
@@ -2247,8 +2240,7 @@  add_cfis_to_fde (void)
 
 	      /* Set the location counter to the new label.  */
 	      xcfi = new_cfi ();
-	      xcfi->dw_cfi_opc = (first ? DW_CFA_set_loc
-				  : DW_CFA_advance_loc4);
+	      xcfi->dw_cfi_opc = DW_CFA_advance_loc4;
 	      xcfi->dw_cfi_oprnd1.dw_cfi_addr = label;
 	      vec_safe_push (fde->dw_fde_cfi, xcfi);
 
@@ -2263,7 +2255,6 @@  add_cfis_to_fde (void)
 	      insn = NEXT_INSN (insn);
 	    }
 	  while (insn != next);
-	  first = false;
 	}
     }
 }