Message ID | 6FABA122-72B5-4F9B-8573-FCFF1481700C@sandoe-acoustics.co.uk |
---|---|
State | New |
Headers | show |
On Sun, Aug 15, 2010 at 08:53:31PM +0100, IainS wrote: > On 14 Aug 2010, at 00:27, Richard Henderson wrote: > >> It also seems like it would >> be a good idea to add a SPEC entry to disable compact unwind if the >> user explicitly uses -fasynchronous-unwind-info. > > At present, I'm going to leave the compact unwinder disabled on > Darwin10, it seems from the testing done by Jack that there are a bunch > of other (probably non-gnu) issues to iron out on that yet. Ergo, the > spec is not needed yet. > Iain, On x86_64-apple-darwin10, I am seeing the following new failures compard to your previous version of this patch... FAIL: g++.dg/eh/async-unwind1.C (test for excess errors) FAIL: g++.dg/eh/async-unwind2.C (test for excess errors) These appear as... Executing on host: /sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/gcc/testsuite/g++/../../g++ -B/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/gcc/testsuite/g++/../../ /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100815/gcc/testsuite/g++.dg/eh/async-unwind1.C -nostdinc++ -I/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/x86_64-apple-darwin10.5.0/i386/libstdc++-v3/include/x86_64-apple-darwin10.5.0 -I/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/x86_64-apple-darwin10.5.0/i386/libstdc++-v3/include -I/sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100815/libstdc++-v3/libsupc++ -I/sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100815/libstdc++-v3/include/backward -I/sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100815/libstdc++-v3/testsuite/util -fmessage-length=0 -Os -fasynchronous-unwind-tables -mpreferred-stack-boundary=4 -L/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/x86_64-apple-darwin10.5.0/i386/libstdc++-v3/src/.libs -B/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/x86_64-apple-darwin10.5.0/i386/libstdc++-v3/src/.libs -L/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/x86_64-apple-darwin10.5.0/i386/libstdc++-v3/src/.libs -L/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/x86_64-apple-darwin10.5.0/i386/libiberty -multiply_defined suppress -lm -m32 -o ./async-unwind1.exe (timeout = 300) /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100815/gcc/testsuite/g++.dg/eh/async-unwind1.C:1:0: warning: this architecture does not fully support -fasynchronous-unwind-tables [-Wall] output is: /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100815/gcc/testsuite/g++.dg/eh/async-unwind1.C:1:0: warning: this architecture does not fully support -fasynchronous-unwind-tables [-Wall] FAIL: g++.dg/eh/async-unwind1.C (test for excess errors) Excess errors: /sw/src/fink.build/gcc46-4.6.0-1000/gcc-4.6-20100815/gcc/testsuite/g++.dg/eh/async-unwind1.C:1:0: warning: this architecture does not fully support -fasynchronous-unwind-tables [-Wall] Setting LD_LIBRARY_PATH to .:/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/x86_64-apple-darwin10.5.0/i386/libstdc++-v3/src/.libs:/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/x86_64-apple-darwin10.5.0/i386/libstdc++-v3/src/.libs:/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/gcc:.:/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/x86_64-apple-darwin10.5.0/i386/libstdc++-v3/src/.libs:/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/x86_64-apple-darwin10.5.0/i386/libstdc++-v3/src/.libs:/sw/src/fink.build/gcc46-4.6.0-1000/darwin_objdir/gcc PASS: g++.dg/eh/async-unwind1.C execution test so while we have a warning, the generated code works fine. I think that as long as we stick with the compatibility unwinder, the asynchronous unwind should be fine (since the compatibility unwinder should functionally be the same as that from gcc 4.2.1 in darwin10). Or do you have some other reason to suspect that there are problems with asynchronous unwind and the compatibility unwinder in darwin10? Jack ps I don't think Richard's suggestion of a SPEC entry to disable compact unwind if the user explicitly uses -fasynchronous-unwind-info makes sense. The use of -no_compact_unwind causes the linked code to use the compatibility unwinder (derived from libgcc in gcc 4.2.1). If you added such a feature, the libjava shared libraries would be linked expecting to run on the compatibility unwinder but additional code compiled with the -fasynchronous-unwind-info flag would suddenly switch FSF gcc over to the compact unwinder. It gives me a headache just trying to imagine what that might do.
Iain, I am also seeing the failure... FAIL: gfortran.dg/g77/980701-0.f -Os execution test at -m64 under this second revision of your _eh darwin patch. This represents a regression from both current gcc trunk and your previous _eh darwin patch... http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01037.html when I tested against either the compatibility or compact unwinders on x86_64-apple-darwin10 or against the compatibility unwinder on i386-apple-darwin10. http://gcc.gnu.org/ml/gcc-testresults/2010-08/msg01454.html http://gcc.gnu.org/ml/gcc-testresults/2010-08/msg01429.html http://gcc.gnu.org/ml/gcc-testresults/2010-08/msg01573.html It would appear that the second patch doesn't exactly reproduce the behavior of the first. Can you break up the changes between the first and second patches into sections to figure out which part triggered the gfortran testsuite failure? Jack
On 08/15/2010 12:53 PM, IainS wrote: > - enum dwarf_call_frame_info dw_cfi_opc; > - dw_cfi_oprnd GTY ((desc ("dw_cfi_oprnd1_desc (%1.dw_cfi_opc)"))) > + unsigned int dw_cfi_opc; > + dw_cfi_oprnd GTY ((desc ("dw_cfi_oprnd1_desc ((unsigned int)%1.dw_cfi_opc)"))) The cast here is useless. > @@ -850,6 +868,7 @@ add_fde_cfi (const char *label, dw_cfi_ref cfi) > case DW_CFA_def_cfa_sf: > case DW_CFA_def_cfa_expression: > case DW_CFA_restore_state: > + case DW_CFA_INTERNAL_start_epilogue: Why? You've already handled that at the top of the function. > + else if (cfi->dw_cfi_opc == DW_CFA_INTERNAL_start_epilogue) > + { > + /* This is a nop unless we want a debug message. */ > + if (flag_debug_asm) > + fputs (ASM_COMMENT_START"\t\t\t" ASM_COMMENT_START > + " DW_CFA_INTERNAL_start_epilogue\n",asm_out_file); Honestly I don't think you should bother printing these in the final version. > + while (cfi->dw_cfi_next) > + /* Skip to the end. */ > + cfi = cfi->dw_cfi_next; It's better to move the comment above the while. As it is, it's hard to immediately see that the while has one statement. -- Also, I'm noticing that dwarf2out.c uses flag_asynchronous_unwind_tables in places where it really means cfun->can_throw_non_call_exceptions. In fact, almost all occurrences of f_a_u_t in dwarf2out.c are in error. The only exception that I can see off-hand is in fde_needed_for_eh_p, which would need an extra check vs cfun->can_throw_non_call_exceptions. All of which affects the changes that you need to make within toplev.c. r~
Hi Richard, On 16 Aug 2010, at 17:18, Richard Henderson wrote: <snipped > .... other points taken on board.... > Also, I'm noticing that dwarf2out.c uses > flag_asynchronous_unwind_tables > in places where it really means cfun->can_throw_non_call_exceptions. > In fact, almost all occurrences of f_a_u_t in dwarf2out.c are in > error. > The only exception that I can see off-hand is in fde_needed_for_eh_p, > which would need an extra check vs cfun- > >can_throw_non_call_exceptions. How should we proceed on this? And also there is the remaining question about the GNU-specific code (which I fear will get re-enabled when the stuff above is fixed). ... should I expect a dwarf-2-strict target to be able to interpret that code? ... or should I be able to replace it with a (possibly less efficient) sequence that is dwarf-2-strict? > All of which affects the changes that you need to make within > toplev.c. OK - although (independent of this patch) it seems to me that toplev.c is overriding a user flag without any diagnostic. thanks for reviewing, Iain
On 08/16/2010 09:38 AM, IainS wrote: > Hi Richard, > > On 16 Aug 2010, at 17:18, Richard Henderson wrote: > > <snipped > .... other points taken on board.... > >> Also, I'm noticing that dwarf2out.c uses >> flag_asynchronous_unwind_tables in places where it really means >> cfun->can_throw_non_call_exceptions. In fact, almost all >> occurrences of f_a_u_t in dwarf2out.c are in error. The only >> exception that I can see off-hand is in fde_needed_for_eh_p, which >> would need an extra check vs cfun->can_throw_non_call_exceptions. > > How should we proceed on this? Patch dwarf2out.c? How else? > And also there is the remaining question about the GNU-specific code > (which I fear will get re-enabled when the stuff above is fixed). What remaining question? What GNU-specific code? r~
Index: gcc/dwarf2out.c =================================================================== --- gcc/dwarf2out.c (revision 163267) +++ gcc/dwarf2out.c (working copy) @@ -268,12 +272,20 @@ typedef union GTY(()) dw_cfi_oprnd_struct { } dw_cfi_oprnd; +/* Use the first out-of-band opcode number as a marker for the start of + epilogues. */ +#define DW_CFA_INTERNAL_start_epilogue 0x100 +#define CFI_T enum dwarf_call_frame_info + +static enum dw_cfi_oprnd_type dw_cfi_oprnd1_desc (unsigned int cfi); +static enum dw_cfi_oprnd_type dw_cfi_oprnd2_desc (unsigned int cfi); + typedef struct GTY(()) dw_cfi_struct { dw_cfi_ref dw_cfi_next; - enum dwarf_call_frame_info dw_cfi_opc; - dw_cfi_oprnd GTY ((desc ("dw_cfi_oprnd1_desc (%1.dw_cfi_opc)"))) + unsigned int dw_cfi_opc; + dw_cfi_oprnd GTY ((desc ("dw_cfi_oprnd1_desc ((unsigned int)%1.dw_cfi_opc)"))) dw_cfi_oprnd1; - dw_cfi_oprnd GTY ((desc ("dw_cfi_oprnd2_desc (%1.dw_cfi_opc)"))) + dw_cfi_oprnd GTY ((desc ("dw_cfi_oprnd2_desc ((unsigned int)%1.dw_cfi_opc)"))) dw_cfi_oprnd2; } dw_cfi_node; @@ -821,9 +833,15 @@ add_fde_cfi (const char *label, dw_cfi_ref cfi) } list_head = &cie_cfi_head; - - if (dwarf2out_do_cfi_asm ()) + + if (cfi->dw_cfi_opc == DW_CFA_INTERNAL_start_epilogue) { + dw_fde_ref fde = current_fde (); + gcc_assert (fde != NULL); + list_head = &fde->dw_fde_cfi; + } + else if (dwarf2out_do_cfi_asm ()) + { if (label) { dw_fde_ref fde = current_fde (); @@ -850,6 +868,7 @@ add_fde_cfi (const char *label, dw_cfi_ref cfi) case DW_CFA_def_cfa_sf: case DW_CFA_def_cfa_expression: case DW_CFA_restore_state: + case DW_CFA_INTERNAL_start_epilogue: if (*label == 0 || strcmp (label, "<do not output>") == 0) label = dwarf2out_cfi_label (true); @@ -2894,12 +2913,22 @@ dwarf2out_cfi_begin_epilogue (rtx insn) return; /* Otherwise, search forward to see if the return insn was the last - basic block of the function. If so, we don't need save/restore. */ + basic block of the function. If so, we don't need save/restore. + However, we do mark the position so that we can skip the epilogue + in _eh frames where required. */ gcc_assert (i != NULL); i = next_real_insn (i); if (i == NULL) - return; + { + dw_cfi_ref cfi_epi_start; + /* Emit a marker for the epilogue start. */ + cfi_epi_start = new_cfi (); + cfi_epi_start->dw_cfi_opc = DW_CFA_INTERNAL_start_epilogue; + add_fde_cfi ("", cfi_epi_start); + return; + } + /* Insert the restore before that next real insn in the stream, and before a potential NOTE_INSN_EPILOGUE_BEG -- we do need these notes to be properly nested. This should be after any label or alignment. This @@ -2941,11 +2970,9 @@ dwarf2out_frame_debug_restore_state (void) } /* Describe for the GTY machinery what parts of dw_cfi_oprnd1 are used. */ -static enum dw_cfi_oprnd_type dw_cfi_oprnd1_desc - (enum dwarf_call_frame_info cfi); static enum dw_cfi_oprnd_type -dw_cfi_oprnd1_desc (enum dwarf_call_frame_info cfi) +dw_cfi_oprnd1_desc (unsigned int cfi) { switch (cfi) { @@ -2953,6 +2980,7 @@ static enum dw_cfi_oprnd_type case DW_CFA_GNU_window_save: case DW_CFA_remember_state: case DW_CFA_restore_state: + case DW_CFA_INTERNAL_start_epilogue: return dw_cfi_oprnd_unused; case DW_CFA_set_loc: @@ -2990,11 +3018,9 @@ static enum dw_cfi_oprnd_type } /* Describe for the GTY machinery what parts of dw_cfi_oprnd2 are used. */ -static enum dw_cfi_oprnd_type dw_cfi_oprnd2_desc - (enum dwarf_call_frame_info cfi); static enum dw_cfi_oprnd_type -dw_cfi_oprnd2_desc (enum dwarf_call_frame_info cfi) +dw_cfi_oprnd2_desc (unsigned int cfi) { switch (cfi) { @@ -3121,6 +3148,13 @@ output_cfi (dw_cfi_ref cfi, dw_fde_ref fde, int fo dw2_asm_output_data (1, (cfi->dw_cfi_opc | (r & 0x3f)), "DW_CFA_restore, column %#lx", r); } + else if (cfi->dw_cfi_opc == DW_CFA_INTERNAL_start_epilogue) + { + /* This is a nop unless we want a debug message. */ + if (flag_debug_asm) + fputs (ASM_COMMENT_START"\t\t\t" ASM_COMMENT_START + " DW_CFA_INTERNAL_start_epilogue\n",asm_out_file); + } else { dw2_asm_output_data (1, cfi->dw_cfi_opc, @@ -3303,6 +3337,13 @@ output_cfi_directive (dw_cfi_ref cfi) cfi->dw_cfi_oprnd1.dw_cfi_offset); break; + case DW_CFA_INTERNAL_start_epilogue: + /* no-op apart from an informational message. */ + if (flag_debug_asm) + fputs (ASM_COMMENT_START"\t\t\t" ASM_COMMENT_START + " DW_CFA_INTERNAL_start_epilogue\n",asm_out_file); + break; + case DW_CFA_remember_state: fprintf (asm_out_file, "\t.cfi_remember_state\n"); break; @@ -3498,6 +3539,46 @@ output_cfis (dw_cfi_ref cfi, bool do_cfi_asm, dw_f } } +/* Output cfi skipping save/restore and epilogues in _eh frames + for targets that do not want them. */ + +static dw_cfi_ref +emit_cfi_or_skip_epilogue (dw_cfi_ref cfi, dw_fde_ref fde, bool for_eh) +{ + if (for_eh + && !flag_asynchronous_unwind_tables) + { + if (cfi->dw_cfi_opc == DW_CFA_remember_state) + { + if (flag_debug_asm) + fputs (ASM_COMMENT_START"\t\t\t"ASM_COMMENT_START + " DW_CFA_remember/restore_state pair skipped\n",asm_out_file); + /* Skip to the restore, unless there's an error and we fall off + the end. */ + while (cfi->dw_cfi_next + && cfi->dw_cfi_opc != DW_CFA_restore_state) + cfi = cfi->dw_cfi_next; + return cfi; + } + if (cfi->dw_cfi_opc == DW_CFA_INTERNAL_start_epilogue) + { + if (flag_debug_asm) + fputs (ASM_COMMENT_START"\t\t\t"ASM_COMMENT_START + " DW_CFA_INTERNAL_start_epilogue\n",asm_out_file); + while (cfi->dw_cfi_next) + /* Skip to the end. */ + cfi = cfi->dw_cfi_next; + return cfi; + } + } + + /* If it's not a special case, then just carry on. + This will also cause the no-op 'DW_CFA_INTERNAL_start_epilogue' to be + listed when flag_debug_asm is set. */ + output_cfi (cfi, fde, for_eh); + return cfi; +} + /* Output one FDE. */ static void @@ -3613,13 +3694,13 @@ output_fde (dw_fde_ref fde, bool for_eh, bool seco fde->dw_fde_current_label = begin; if (!fde->dw_fde_switched_sections) for (cfi = fde->dw_fde_cfi; cfi != NULL; cfi = cfi->dw_cfi_next) - output_cfi (cfi, fde, for_eh); + cfi = emit_cfi_or_skip_epilogue (cfi, fde, for_eh); else if (!second) { if (fde->dw_fde_switch_cfi) for (cfi = fde->dw_fde_cfi; cfi != NULL; cfi = cfi->dw_cfi_next) { - output_cfi (cfi, fde, for_eh); + cfi = emit_cfi_or_skip_epilogue (cfi, fde, for_eh); if (cfi == fde->dw_fde_switch_cfi) break; } @@ -3627,7 +3708,6 @@ output_fde (dw_fde_ref fde, bool for_eh, bool seco else { dw_cfi_ref cfi_next = fde->dw_fde_cfi; - if (fde->dw_fde_switch_cfi) { cfi_next = fde->dw_fde_switch_cfi->dw_cfi_next; @@ -3636,7 +3716,7 @@ output_fde (dw_fde_ref fde, bool for_eh, bool seco fde->dw_fde_switch_cfi->dw_cfi_next = cfi_next; } for (cfi = cfi_next; cfi != NULL; cfi = cfi->dw_cfi_next) - output_cfi (cfi, fde, for_eh); + cfi = emit_cfi_or_skip_epilogue (cfi, fde, for_eh); } /* If we are to emit a ref/link from function bodies to their frame tables, Index: gcc/toplev.c =================================================================== --- gcc/toplev.c (revision 163267) +++ gcc/toplev.c (working copy) @@ -1798,9 +1801,9 @@ process_options (void) if (flag_rename_registers == AUTODETECT_VALUE) flag_rename_registers = flag_unroll_loops || flag_peel_loops; - if (flag_non_call_exceptions) + if (flag_non_call_exceptions && flag_asynchronous_unwind_tables == 2) flag_asynchronous_unwind_tables = 1; - if (flag_asynchronous_unwind_tables) + if (flag_asynchronous_unwind_tables || flag_non_call_exceptions) flag_unwind_tables = 1; if (flag_value_profile_transformations) Index: gcc/config/darwin.c =================================================================== --- gcc/config/darwin.c (revision 163267) +++ gcc/config/darwin.c (working copy) @@ -1884,7 +1915,22 @@ darwin_override_options (void) flag_reorder_blocks_and_partition = 0; flag_reorder_blocks = 1; } + + if (flag_asynchronous_unwind_tables) + { + if (flag_asynchronous_unwind_tables == 2) + flag_asynchronous_unwind_tables = 0; + else + /* Issue a warning */ + warning (OPT_Wall, + "this architecture does not fully support" + " -fasynchronous-unwind-tables"); + } + if ((flag_exceptions || flag_non_call_exceptions) + && strverscmp (darwin_macosx_version_min, "10.4") >= 0) + flag_unwind_tables = 1; + if (flag_mkernel || flag_apple_kext) { /* -mkernel implies -fapple-kext for C++ */ @@ -1897,6 +1943,9 @@ darwin_override_options (void) flag_exceptions = 0; /* No -fnon-call-exceptions data in kexts. */ flag_non_call_exceptions = 0; + /* so no tables either.. */ + flag_unwind_tables = 0; + flag_asynchronous_unwind_tables = 0; /* We still need to emit branch islands for kernel context. */ darwin_emit_branch_islands = true; }