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

[Uros Bizjak]

On Mon, Oct 23, 2017 at 10:04 AM, Richard Biener  wrote:
> On Mon, 23 Oct 2017, Jakub Jelinek wrote:
>
>> On Mon, Oct 23, 2017 at 09:48:50AM +0200, Richard Biener wrote:
>> > > --- gcc/targhooks.c.jj2017-10-13 19:02:08.0 +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?
>>
>> This hook is called on the argument of CONST or SYMBOL_REF.
>> UNSPEC_VOLATILE can't appear inside of CONST, it wouldn't be CONST then.
>>
>> UNSPEC appearing outside of CONST is rejected unconditionally in
>> mem_loc_descriptor:
>> ...
>> case UNSPEC:
>> ...
>>   /* If delegitimize_address couldn't do anything with the UNSPEC, we
>>  can't express it in the debug info.  This can happen e.g. with some
>>  TLS UNSPECs.  */
>>   break;
>> and for UNSPEC_VOLATILE we just ICE, because var-tracking shouldn't let
>> those through:
>> default:
>>   if (flag_checking)
>> {
>>   print_rtl (stderr, rtl);
>>   gcc_unreachable ();
>> }
>>   break;
>
> Ok.  The patch looks fine from a middle-end point of view.

LGTM for the x86 part.

Thanks,
Uros.

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

[Richard Biener]

On Mon, 23 Oct 2017, Jakub Jelinek wrote:

> On Mon, Oct 23, 2017 at 09:48:50AM +0200, Richard Biener wrote:
> > > --- gcc/targhooks.c.jj2017-10-13 19:02:08.0 +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?
> 
> This hook is called on the argument of CONST or SYMBOL_REF.
> UNSPEC_VOLATILE can't appear inside of CONST, it wouldn't be CONST then.
> 
> UNSPEC appearing outside of CONST is rejected unconditionally in
> mem_loc_descriptor:
> ...
> case UNSPEC:
> ...
>   /* If delegitimize_address couldn't do anything with the UNSPEC, we
>  can't express it in the debug info.  This can happen e.g. with some
>  TLS UNSPECs.  */
>   break;
> and for UNSPEC_VOLATILE we just ICE, because var-tracking shouldn't let
> those through:
> default:
>   if (flag_checking)
> {
>   print_rtl (stderr, rtl);
>   gcc_unreachable ();
> }
>   break;

Ok.  The patch looks fine from a middle-end point of view.

Thanks,
Richard.

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

[Jakub Jelinek]

On Mon, Oct 23, 2017 at 09:48:50AM +0200, Richard Biener wrote:
> > --- gcc/targhooks.c.jj  2017-10-13 19:02:08.0 +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?

This hook is called on the argument of CONST or SYMBOL_REF.
UNSPEC_VOLATILE can't appear inside of CONST, it wouldn't be CONST then.

UNSPEC appearing outside of CONST is rejected unconditionally in
mem_loc_descriptor:
...
case UNSPEC:
...
  /* If delegitimize_address couldn't do anything with the UNSPEC, we
 can't express it in the debug info.  This can happen e.g. with some
 TLS UNSPECs.  */
  break;
and for UNSPEC_VOLATILE we just ICE, because var-tracking shouldn't let
those through:
default:
  if (flag_checking)
{
  print_rtl (stderr, rtl);
  gcc_unreachable ();
}
  break;

Jakub

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

[Richard Biener]

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  
> 
>   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.0 +0200
> +++ gcc/target.def2017-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.jj2017-10-13 19:02:08.0 +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.jj2017-10-13 19:02:08.0 +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.jj2017-10-19 16:18:44.0 +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 

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

[Jakub Jelinek]

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  

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.0 +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.0 +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)
+return true;
+  return false;
+}
+
 rtx
 default_expand_builtin_saveregs (void)
 {
--- gcc/targhooks.h.jj  2017-10-13 19:02:08.0 +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.0 +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