Re: [PATCH] Fix debug for -mcall-ms2sysv-xlogues stubs fallout (PR target/83917)

[Jeff Law]

On 02/27/2018 01:29 AM, Jakub Jelinek wrote:
> On Mon, Feb 26, 2018 at 08:05:56PM -0600, Daniel Santos wrote:
> --- libgcc/config/i386/cygwin.S.jj2018-01-03 10:42:56.309763515 
> +0100
> +++ libgcc/config/i386/cygwin.S   2018-02-22 15:30:34.597925496 +0100
> @@ -23,31 +23,13 @@
>   * .
>   */
>  
> -#include "auto-host.h"
 The following include should be here.

 +#include "i386-asm.h"
>>> I don't understand this.  i386-asm.h needs (both before my patch and after
>>> it) both auto-host.h and auto-target.h, as it tests
>>> HAVE_GAS_SECTIONS_DIRECTIVE (this one newly, comes from cygwin.S)
>>
>> The problem is that HAVE_GAS_SECTIONS_DIRECTIVE gets defined (or not) in
>> ../../gcc/auto-host.h, but you are testing it before including
>> auto-host.h, either directly or via i386-asm.h.  So if i386-asm.h
>> depends upon HAVE_GAS_SECTIONS_DIRECTIVE first being defined then it is
>> a circular dependency.
>>
>> In its current form, cygwin.S would never define USE_GAS_CFI_DIRECTIVES
>> prior to including i386-asm.h and also never emit
>>     .cfi_sections    .debug_frame
>> and rather or not USE_GAS_CFI_DIRECTIVES ends up being defined to 1 or 0
>> depends upon the test of __GCC_HAVE_DWARF2_CFI_ASM in i386-asm.h.
> 
> Ugh, you're right.  I was trying to preserve existing behavior for cygwin.S,
> but failed to do so.  Unfortunately the patch which added this stuff from
> Kai T. and Richard H. from 2010 is not in gcc-patches archives; in any case,
> I think nothing seriously bad happens if with older gas versions which do
> support .cfi_* directives but not .cfi_sections .debug_frame we emit the CFI
> into .eh_frame section rather than .debug_frame.
> 
> So this patch simplifies it, with only one guard for the non-trivial
> vs. trivial cfi_* definitions (based on whether GCC itself would use it)
> and only guard the .cfi_sections directive on whether it is really
> available.
> 
> The __GCC_HAVE_DWARF2_CFI_ASM definition actually sometimes depends on the
> .cfi_sections presence too:
>   /* If we can't get the assembler to emit only .debug_frame, and we don't 
> need
>  dwarf2 unwind info for exceptions, then emit .debug_frame by hand.  */
>   if (!HAVE_GAS_CFI_SECTIONS_DIRECTIVE && !dwarf2out_do_eh_frame ())
> return false;
> but doesn't actually guarantee it always, as when doing .eh_frame it will
> not require .cfi_sections.
> 
> This spot brings in another, preexisting bug in cygwin.S though - 
> the HAVE_GAS_CFI_SECTIONS_DIRECTIVE macro is always defined, to 0 or 1,
> rather than sometimes #define and sometines #undef.
> 
>> Ultimately, the proper cleanup will be moving these tests out of
>> {gcc,libgcc}/configure.ac and into .m4 files in the root config
>> directory so that we don't uglify them with massive copy & pastes. 
>> These tests are also fairly complex as there are a lot of dependencies. 
>> m4 isn't my strong suite, but I can look at this after we're out of code
>> freeze.
> 
> Not really sure about that, because we really want to do a different thing
> in gcc/configure.ac (need to test the assembler directly, use
> GCC_TARGET_TEMPLATE) while in libgcc it does usually something different.
> 
> The libgcc configure already has all the code for the .hidden directive,
> as it uses it too, just it is only a pair of AC_SUBSTs rather than
> AC_DEFINE_UNQUOTED.
> The test for HAVE_GAS_CFI_SECTIONS_DIRECTIVE alternative can be compile
> int foo (int, char *);
> int bar (int x) { char *y = __builtin_alloca (x); return foo (x + 1, y) + 1; }
> with -g -fno-asynchronous-unwind-tables -fno-unwind-tables -fno-exceptions
> and scan for .cfi_sections .debug_frame.
> 
> So here is a new (I've committed the previous patch since then), only lightly
> tested (only on x86_64-linux and don't have too old binutils around), patch:
> 
> 2018-02-27  Jakub Jelinek  
> 
>   PR debug/83917
>   * configure.ac (AS_HIDDEN_DIRECTIVE): AC_DEFINE_UNQUOTED this to
>   $asm_hidden_op if visibility ("hidden") attribute works.
>   (HAVE_AS_CFI_SECTIONS): New AC_DEFINE.
>   * config/i386/i386-asm.h: Don't include auto-host.h.
>   (PACKAGE_VERSION, PACKAGE_NAME, PACKAGE_STRING, PACKAGE_TARNAME,
>   PACKAGE_URL): Don't undefine.
>   (USE_GAS_CFI_DIRECTIVES): Don't use nor define this macro, instead
>   guard cfi_startproc only on ifdef __GCC_HAVE_DWARF2_CFI_ASM.
>   (FN_HIDDEN): Change guard from #ifdef HAVE_GAS_HIDDEN to
>   #ifdef AS_HIDDEN_DIRECTIVE, use AS_HIDDEN_DIRECTIVE macro in the
>   definition instead of hardcoded .hidden.
>   * config/i386/cygwin.S: Include i386-asm.h first before .cfi_sections
>   directive.  Use #ifdef HAVE_AS_CFI_SECTIONS rather than
>   #ifdef HAVE_GAS_CFI_SECTIONS_DIRECTIVE to guard .cfi_sections.
>   (USE_GAS_CFI_DIRECTIVES): Don't define.
>   * configure: Regenerated.
>   * config.in: Likewise.
OK.
jeff

Re: [PATCH] Fix debug for -mcall-ms2sysv-xlogues stubs fallout (PR target/83917)

[Jakub Jelinek]

On Tue, Feb 27, 2018 at 09:29:36AM +0100, Jakub Jelinek wrote:
> So here is a new (I've committed the previous patch since then), only lightly
> tested (only on x86_64-linux and don't have too old binutils around), patch:
> 
> 2018-02-27  Jakub Jelinek  
> 
>   PR debug/83917
>   * configure.ac (AS_HIDDEN_DIRECTIVE): AC_DEFINE_UNQUOTED this to
>   $asm_hidden_op if visibility ("hidden") attribute works.
>   (HAVE_AS_CFI_SECTIONS): New AC_DEFINE.
>   * config/i386/i386-asm.h: Don't include auto-host.h.
>   (PACKAGE_VERSION, PACKAGE_NAME, PACKAGE_STRING, PACKAGE_TARNAME,
>   PACKAGE_URL): Don't undefine.
>   (USE_GAS_CFI_DIRECTIVES): Don't use nor define this macro, instead
>   guard cfi_startproc only on ifdef __GCC_HAVE_DWARF2_CFI_ASM.
>   (FN_HIDDEN): Change guard from #ifdef HAVE_GAS_HIDDEN to
>   #ifdef AS_HIDDEN_DIRECTIVE, use AS_HIDDEN_DIRECTIVE macro in the
>   definition instead of hardcoded .hidden.
>   * config/i386/cygwin.S: Include i386-asm.h first before .cfi_sections
>   directive.  Use #ifdef HAVE_AS_CFI_SECTIONS rather than
>   #ifdef HAVE_GAS_CFI_SECTIONS_DIRECTIVE to guard .cfi_sections.
>   (USE_GAS_CFI_DIRECTIVES): Don't define.
>   * configure: Regenerated.
>   * config.in: Likewise.

Now successfully bootstrapped/regtested on x86_64-linux and i686-linux.

Jakub