Re: [PATCH] Fix wrong-debug with i?86/x86_64 _GLOBAL_OFFSET_TABLE_ (PR debug/82630)

[Richard Biener <rguenther at suse dot de>]

On Mon, 23 Oct 2017, Jakub Jelinek wrote:

> Hi!
> 
> If all fails, when we can't prove that the PIC register is in some hard
> register, we delegitimize something + foo@GOTOFF as (something -
> _GLOBAL_OFFSET_TABLE_) + foo.  That is reasonable for the middle-end to
> understand what is going on (it will never match in actual instructions
> though), unfortunately when trying to emit that into .debug_info section
> we run into the problem that .long _GLOBAL_OFFSET_TABLE_ etc. is not
> actually assembled as address of _GLOBAL_OFFSET_TABLE_, but as
> _GLOBAL_OFFSET_TABLE_-. (any time the assembler sees _GLOBAL_OFFSET_TABLE_
> symbol by name, it adds the special relocation) and thus we get a bogus
> expression.
> 
> I couldn't come up with a way to express this that wouldn't be even larger
> than what we have, but if we actually not delegitimize it at all and let
> it be emitted as
>   something
>   .byte DW_OP_addr
>   .long foo@GOTOFF
>   .byte DW_OP_plus
> then it works fine and is even shorter than what we used to emit -
>   something
>   .byte DW_OP_addr
>   .long _GLOBAL_OFFSET_TABLE_
>   .byte DW_OP_minus
>   .byte DW_OP_addr
>   .long foo
>   .byte DW_OP_plus
> In order to achieve that, we need to allow selected UNSPECs through
> into debug info, current trunk just gives up on all UNSPECs.
> 
> Fortunately, we already have a hook for rejecting some constants, so
> by adding the rejection of all UNSPECs into the hook and on i386 overriding
> that hook we achieve what we want.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2017-10-23  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR debug/82630
> 	* target.def (const_not_ok_for_debug_p): Default to
> 	default_const_not_ok_for_debug_p instead of hook_bool_rtx_false.
> 	* targhooks.h (default_const_not_ok_for_debug_p): New declaration.
> 	* targhooks.c (default_const_not_ok_for_debug_p): New function.
> 	* dwarf2out.c (const_ok_for_output_1): Only reject UNSPECs for
> 	which targetm.const_not_ok_for_debug_p returned true.
> 	* config/arm/arm.c (arm_const_not_ok_for_debug_p): Return true
> 	for UNSPECs.
> 	* config/powerpcspe/powerpcspe.c (rs6000_const_not_ok_for_debug_p):
> 	Likewise.
> 	* config/rs6000/rs6000.c (rs6000_const_not_ok_for_debug_p): Likewise.
> 	* config/i386/i386.c (ix86_delegitimize_address_1): Don't delegitimize
> 	UNSPEC_GOTOFF with addend into addend - _GLOBAL_OFFSET_TABLE_ + symbol
> 	if !base_term_p.
> 	(ix86_const_not_ok_for_debug_p): New function.
> 	(i386_asm_output_addr_const_extra): Handle UNSPEC_GOTOFF.
> 	(TARGET_CONST_NOT_OK_FOR_DEBUG_P): Redefine.
> 
> 	* g++.dg/guality/pr82630.C: New test.
> 
> --- gcc/target.def.jj	2017-10-10 11:54:13.000000000 +0200
> +++ gcc/target.def	2017-10-20 14:07:06.463135128 +0200
> @@ -2822,7 +2822,7 @@ DEFHOOK
>   "This hook should return true if @var{x} should not be emitted into\n\
>  debug sections.",
>   bool, (rtx x),
> - hook_bool_rtx_false)
> + default_const_not_ok_for_debug_p)
>  
>  /* Given an address RTX, say whether it is valid.  */
>  DEFHOOK
> --- gcc/targhooks.c.jj	2017-10-13 19:02:08.000000000 +0200
> +++ gcc/targhooks.c	2017-10-20 14:26:07.945464025 +0200
> @@ -177,6 +177,14 @@ default_legitimize_address_displacement
>    return false;
>  }
>  
> +bool
> +default_const_not_ok_for_debug_p (rtx x)
> +{
> +  if (GET_CODE (x) == UNSPEC)

What about UNSPEC_VOLATILE?

> +    return true;
> +  return false;
> +}
> +
>  rtx
>  default_expand_builtin_saveregs (void)
>  {
> --- gcc/targhooks.h.jj	2017-10-13 19:02:08.000000000 +0200
> +++ gcc/targhooks.h	2017-10-20 14:26:07.945464025 +0200
> @@ -26,6 +26,7 @@ extern void default_external_libcall (rt
>  extern rtx default_legitimize_address (rtx, rtx, machine_mode);
>  extern bool default_legitimize_address_displacement (rtx *, rtx *,
>  						     machine_mode);
> +extern bool default_const_not_ok_for_debug_p (rtx);
>  
>  extern int default_unspec_may_trap_p (const_rtx, unsigned);
>  extern machine_mode default_promote_function_mode (const_tree, machine_mode,
> --- gcc/dwarf2out.c.jj	2017-10-19 16:18:44.000000000 +0200
> +++ gcc/dwarf2out.c	2017-10-20 14:39:49.432647598 +0200
> @@ -13740,9 +13740,17 @@ expansion_failed (tree expr, rtx rtl, ch
>  static bool
>  const_ok_for_output_1 (rtx rtl)
>  {
> -  if (GET_CODE (rtl) == UNSPEC)
> +  if (targetm.const_not_ok_for_debug_p (rtl))
>      {
> -      /* If delegitimize_address couldn't do anything with the UNSPEC, assume
> +      if (GET_CODE (rtl) != UNSPEC)
> +	{
> +	  expansion_failed (NULL_TREE, rtl,
> +			    "Expression rejected for debug by the backend.\n");
> +	  return false;
> +	}
> +
> +      /* If delegitimize_address couldn't do anything with the UNSPEC, and
> +	 the target hook doesn't explicitly allow it in debug info, assume
>  	 we can't express it in the debug info.  */
>        /* Don't complain about TLS UNSPECs, those are just too hard to
>  	 delegitimize.  Note this could be a non-decl SYMBOL_REF such as
> @@ -13769,13 +13777,6 @@ const_ok_for_output_1 (rtx rtl)
>        return false;
>      }
>  
> -  if (targetm.const_not_ok_for_debug_p (rtl))
> -    {
> -      expansion_failed (NULL_TREE, rtl,
> -			"Expression rejected for debug by the backend.\n");
> -      return false;
> -    }
> -
>    /* FIXME: Refer to PR60655. It is possible for simplification
>       of rtl expressions in var tracking to produce such expressions.
>       We should really identify / validate expressions
> --- gcc/config/arm/arm.c.jj	2017-10-19 16:18:44.000000000 +0200
> +++ gcc/config/arm/arm.c	2017-10-20 14:28:06.302046887 +0200
> @@ -30391,6 +30391,8 @@ arm_const_not_ok_for_debug_p (rtx p)
>    tree decl_op0 = NULL;
>    tree decl_op1 = NULL;
>  
> +  if (GET_CODE (p) == UNSPEC)
> +    return true;
>    if (GET_CODE (p) == MINUS)
>      {
>        if (GET_CODE (XEXP (p, 1)) == SYMBOL_REF)
> --- gcc/config/powerpcspe/powerpcspe.c.jj	2017-10-17 22:10:10.000000000 +0200
> +++ gcc/config/powerpcspe/powerpcspe.c	2017-10-20 14:28:40.823633544 +0200
> @@ -9536,6 +9536,8 @@ rs6000_delegitimize_address (rtx orig_x)
>  static bool
>  rs6000_const_not_ok_for_debug_p (rtx x)
>  {
> +  if (GET_CODE (x) == UNSPEC)
> +    return true;
>    if (GET_CODE (x) == SYMBOL_REF
>        && CONSTANT_POOL_ADDRESS_P (x))
>      {
> --- gcc/config/rs6000/rs6000.c.jj	2017-10-17 22:10:10.000000000 +0200
> +++ gcc/config/rs6000/rs6000.c	2017-10-20 14:29:01.066391168 +0200
> @@ -8985,6 +8985,8 @@ rs6000_delegitimize_address (rtx orig_x)
>  static bool
>  rs6000_const_not_ok_for_debug_p (rtx x)
>  {
> +  if (GET_CODE (x) == UNSPEC)
> +    return true;
>    if (GET_CODE (x) == SYMBOL_REF
>        && CONSTANT_POOL_ADDRESS_P (x))
>      {
> --- gcc/config/i386/i386.c.jj	2017-10-20 09:16:10.000000000 +0200
> +++ gcc/config/i386/i386.c	2017-10-20 14:53:19.520953612 +0200
> @@ -16657,13 +16657,17 @@ ix86_delegitimize_address_1 (rtx x, bool
>  	 movl foo@GOTOFF(%ecx), %edx
>  	 in which case we return (%ecx - %ebx) + foo
>  	 or (%ecx - _GLOBAL_OFFSET_TABLE_) + foo if pseudo_pic_reg
> -	 and reload has completed.  */
> +	 and reload has completed.  Don't do the latter for debug,
> +	 as _GLOBAL_OFFSET_TABLE_ can't be expressed in the assembly.  */
>        if (pic_offset_table_rtx
>  	  && (!reload_completed || !ix86_use_pseudo_pic_reg ()))
>          result = gen_rtx_PLUS (Pmode, gen_rtx_MINUS (Pmode, copy_rtx (addend),
>  						     pic_offset_table_rtx),
>  			       result);
> -      else if (pic_offset_table_rtx && !TARGET_MACHO && !TARGET_VXWORKS_RTP)
> +      else if (base_term_p
> +	       && pic_offset_table_rtx
> +	       && !TARGET_MACHO
> +	       && !TARGET_VXWORKS_RTP)
>  	{
>  	  rtx tmp = gen_rtx_SYMBOL_REF (Pmode, GOT_SYMBOL_NAME);
>  	  tmp = gen_rtx_MINUS (Pmode, copy_rtx (addend), tmp);
> @@ -16716,6 +16720,25 @@ ix86_find_base_term (rtx x)
>  
>    return ix86_delegitimize_address_1 (x, true);
>  }
> +
> +/* Return true if X shouldn't be emitted into the debug info.
> +   Disallow UNSPECs other than @gotoff - we can't emit _GLOBAL_OFFSET_TABLE_
> +   symbol easily into the .debug_info section, so we need not to
> +   delegitimize, but instead assemble as @gotoff.
> +   Disallow _GLOBAL_OFFSET_TABLE_ SYMBOL_REF - the assembler magically
> +   assembles that as _GLOBAL_OFFSET_TABLE_-. expression.  */
> +
> +static bool
> +ix86_const_not_ok_for_debug_p (rtx x)
> +{
> +  if (GET_CODE (x) == UNSPEC && XINT (x, 1) != UNSPEC_GOTOFF)
> +    return true;
> +
> +  if (SYMBOL_REF_P (x) && strcmp (XSTR (x, 0), GOT_SYMBOL_NAME) == 0)
> +    return true;
> +
> +  return false;
> +}
>  
>  static void
>  put_condition_code (enum rtx_code code, machine_mode mode, bool reverse,
> @@ -18032,6 +18055,10 @@ i386_asm_output_addr_const_extra (FILE *
>    op = XVECEXP (x, 0, 0);
>    switch (XINT (x, 1))
>      {
> +    case UNSPEC_GOTOFF:
> +      output_addr_const (file, op);
> +      fputs ("@gotoff", file);
> +      break;
>      case UNSPEC_GOTTPOFF:
>        output_addr_const (file, op);
>        /* FIXME: This might be @TPOFF in Sun ld.  */
> @@ -49415,6 +49442,9 @@ ix86_run_selftests (void)
>  #undef TARGET_DELEGITIMIZE_ADDRESS
>  #define TARGET_DELEGITIMIZE_ADDRESS ix86_delegitimize_address
>  
> +#undef TARGET_CONST_NOT_OK_FOR_DEBUG_P
> +#define TARGET_CONST_NOT_OK_FOR_DEBUG_P ix86_const_not_ok_for_debug_p
> +
>  #undef TARGET_MS_BITFIELD_LAYOUT_P
>  #define TARGET_MS_BITFIELD_LAYOUT_P ix86_ms_bitfield_layout_p
>  
> --- gcc/testsuite/g++.dg/guality/pr82630.C.jj	2017-10-20 15:23:37.978128740 +0200
> +++ gcc/testsuite/g++.dg/guality/pr82630.C	2017-10-20 15:30:39.608050350 +0200
> @@ -0,0 +1,58 @@
> +// PR debug/82630
> +// { dg-do run }
> +// { dg-additional-options "-fPIC" { target fpic } }
> +
> +struct C
> +{
> +  int &c;
> +  long d;
> +  __attribute__((always_inline)) C (int &x) : c(x), d() {}
> +};
> +int v;
> +
> +__attribute__((noipa)) void
> +fn1 (const void *x)
> +{
> +  asm volatile ("" : : "g" (x) : "memory");
> +}
> +
> +__attribute__((noipa)) void
> +fn2 (C x)
> +{
> +  int a = x.c + x.d;
> +  asm volatile ("" : : "g" (a) : "memory");
> +}
> +
> +__attribute__((noipa)) void
> +fn3 (void)
> +{
> +  asm volatile ("" : : : "memory");
> +}
> +
> +__attribute__((noipa))
> +#ifdef __i386__
> +__attribute__((regparm (2)))
> +#endif
> +static void
> +fn4 (int *x, const char *y, C z)
> +{
> +  fn2 (C (*x));
> +  fn1 ("baz");
> +  fn2 (z);	// { dg-final { gdb-test 41 "y\[0\]" "'f'" } }
> +  fn1 ("baz");	// { dg-final { gdb-test 41 "y\[1\]" "'o'" } }
> +}
> +
> +__attribute__((noipa)) void
> +fn5 (int *x)
> +{
> +  fn4 (x, "foo", C (*x));
> +  fn3 ();
> +}
> +
> +int
> +main ()
> +{
> +  int a = 10;
> +  fn5 (&a);
> +  return 0;
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)