Re: [PATCH] arc/bfd: Fix segfault from debug code, and disable debug by default

[Cupertino Miranda <Cupertino dot Miranda at synopsys dot com>]

Hi Andrew,

Thank you for the patch, however neither me or Claudiu agree with
reverting the overflow information printing.
Overflow information printing is a basic need for the user to identify
linking/compiling problems.

Until now we were unable to tight this up. Unfortunately, we have other
more important issues/problems to fix.
Nevertheless, your free to improve it and contribute. ;-)

Looking forward for your understanding and help to tight it up.

Best regards,
Cupertino

On 07/13/2016 10:41 PM, Andrew Burgess wrote:
> This patch proposes to revert a recent commit b9316f59852ff821cf621a
> (Enable relocation overflow messages by default):
>
>   https://sourceware.org/ml/binutils/2016-07/msg00074.html
>
> Sorry that I did not see this patch previously otherwise I would have
> raised my objections earlier.  My objections are:
>
>   1. Should be using the einfo / info callbacks on the bfd_link_info
>      object to print messages, not calling fprintf to stderr from
>      within the bfd library.
>
>   2. Messages are not wrapped in _( ... ) for internationalisation.
>
>   3. Leaving the macro being called PR_DEBUG is not nice if it is now
>      on all the time, if we still want to use a macro at all then it
>      should be renamed to something like RELOC_OVERFLOW_MESSAGE, or
>      similar.
>
>   4. Some of the message are not really production ready (in my
>      opinion), for example: "Relocation overflows !!!!".  I've fixed
>      this in the patch below.
>
>   5. Some of this code causes ld to segfault.  Fixed in the patch
>      below.
>
> I've fixed 4 and 5, these are the easier ones.  I've not addressed 1,
> 2 or 3.  Ideally I'd prefer to just silence the debug again by
> default, and if it's really, really wanted, then it can be enabled
> properly again later.
>
> Feedback welcome,
>
> Thanks,
> Andrew
>
>
> ---
>
> The PR_DEBUG macro in arc-elf32.c file prints information about
> relocation errors.  There are a few problems with this macro; first, the
> printing is done using fprintf to stderr, rather than using the
> einfo/info callbacks on the bfd_link_info object.  Second, none of the
> messages are wrapped in _("...") to allow for internationalisation,
> third, one of the debug messages causes a segfault, and finally (and
> this is more opinion) some of the messages should be improved before
> being put into production.
>
> This commit fixes the segfault, and improves one of the
> messages (removing a "!!!").  I've also turned the debug off for now.
> If there's a real desire to have this being produced all of the time
> then the internationalisation and use of einfo/info can be fixed later.
>
> bfd/ChangeLog:
>
> 	* elf32-arc.c (PR_DEBUG): Only define to fprintf when
> 	ARC_ENABLE_DEBUG is defined.
> 	(debug_arc_reloc): Handle symbols that are not from an input file.
> 	(arc_do_relocation): Remove excessive exclamation points.
> ---
>  bfd/ChangeLog   |  7 +++++++
>  bfd/elf32-arc.c | 12 ++++++++----
>  2 files changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/bfd/elf32-arc.c b/bfd/elf32-arc.c
> index ab27d4a..c58013d 100644
> --- a/bfd/elf32-arc.c
> +++ b/bfd/elf32-arc.c
> @@ -29,11 +29,10 @@
>  #include "opcode/arc.h"
>  #include "arc-plt.h"
>  
> -#define PR_DEBUG(fmt, args...) fprintf (stderr, fmt, ##args)
> -
>  /* #define ARC_ENABLE_DEBUG 1  */
>  #ifndef ARC_ENABLE_DEBUG
>  #define ARC_DEBUG(...)
> +#define PR_DEBUG(fmt, args...)
>  #else
>  static char *
>  name_for_global_symbol (struct elf_link_hash_entry *h)
> @@ -45,6 +44,7 @@ name_for_global_symbol (struct elf_link_hash_entry *h)
>      return h->root.root.string;
>  }
>  #define ARC_DEBUG(args...) fprintf (stderr, ##args)
> +#define PR_DEBUG(fmt, args...) fprintf (stderr, fmt, ##args)
>  #endif
>  
>  
> @@ -669,7 +669,11 @@ debug_arc_reloc (struct arc_relocation_data reloc_data)
>  		   ((unsigned int) reloc_data.sym_section->output_section->vma));
>  	}
>        PR_DEBUG ( "\n");
> -      PR_DEBUG ("  file: %s\n", reloc_data.sym_section->owner->filename);
> +      if (reloc_data.sym_section->owner)
> +        {
> +          PR_DEBUG ("  file: %s\n",
> +                    reloc_data.sym_section->owner->filename);
> +        }
>      }
>    else
>      {
> @@ -917,7 +921,7 @@ arc_do_relocation (bfd_byte * contents,
>  #define DEBUG_ARC_RELOC(A) debug_arc_reloc (A)
>    if (flag != bfd_reloc_ok)
>      {
> -      PR_DEBUG ( "Relocation overflows !!!!\n");
> +      PR_DEBUG ("Relocation overflows:\n");
>  
>        DEBUG_ARC_RELOC (reloc_data);
>