Re: [PATCH] wwwdocs: contribute.html: Update consensus on patch content.

[Nick Clifton]

Hi Christophe,


I have a follow-up one: I think the same applies to binutils, but I
don't think any maintainer / contributor expressed an opinion, and
IIUC patch policy for binutils is (lightly) documented at
https://sourceware.org/binutils/wiki/HowToContribute
Maybe Nick can update it? 


Done.


(I don't have such rights)


Would you like them ?  It is easy enough to set up.

Cheers
  Nick

RFC: Top level configure: Require a minimum version 6.8 texinfo

[Nick Clifton via Gcc-patches]

Hi Guys,

  Currently the top level configure.ac file sets the minimum required
  version of texinfo to be 4.7.  I would like to propose changing this
  to 6.8.
  
  The reason for the change is that the bfd documentation now needs at
  least version 6.8 in order to build[1][2].  Given that 4.7 is now
  almost 20 years old (it was released in April 2004), updating the
  requirement to a newer version does seem reasonable.  On the other
  hand 6.8 is quite new (it was released in March 2021), so a lot of
  systems out there may not have it.

  Thoughts ?

Cheers
  Nick

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=30703
[2] https://sourceware.org/pipermail/binutils/2023-February/125943.html

Suggested patch:

diff --git a/configure.ac b/configure.ac
index 01cfd017273..10bfef1c6c5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -3678,10 +3678,10 @@ case " $build_configdirs " in
   *" texinfo "*) MAKEINFO='$$r/$(BUILD_SUBDIR)/texinfo/makeinfo/makeinfo' ;;
   *)
 changequote(,)
-# For an installed makeinfo, we require it to be from texinfo 4.7 or
+# For an installed makeinfo, we require it to be from texinfo 6.8 or
 # higher, else we use the "missing" dummy.
 if ${MAKEINFO} --version \
-   | egrep 'texinfo[^0-9]*(4\.([7-9]|[1-9][0-9])|[5-9]|[1-9][0-9])' 
>/dev/null 2>&1; then
+   | egrep 'texinfo[^0-9]*(6\.([8-9]|[1-9][0-9])|[7-9]|[1-9][0-9])' 
>/dev/null 2>&1; then
   :
 else
   MAKEINFO="$MISSING makeinfo"

Re: [PATCH] msp430: Mark unused attribute

[Nick Clifton via Gcc-patches]

Hi Jan-Benedict,


gcc/ChangeLog:
* config/msp430/msp430.cc (msp430_single_op_cost): Mark unused argument.



Okay for HEAD?


Patch approved - please apply.   (I think that this patch would also count as
an "obvious" fix, but thanks for asking anyway).

Cheers
  Nick

Re: RFA: Another Rust demangler recursion limit

[Nick Clifton via Gcc-patches]

Hi Jeff,

OK. 


Thanks.


And yes, I wish someone else was looking at this stuff.  Rust isn't really on 
my radar right now...


I have been toying with the idea of putting myself forward as a maintainer
for the libiberty sources.  I just wish that I had more free time...

Cheers
  Nick

RFA: Another Rust demangler recursion limit

[Nick Clifton via Gcc-patches]

Hi Jeff,

  [I am sending this to your directly since you seem to be the only one
  reviewing these patches].

  Hot on the heels of the fix for the recursion problem in demangle_const
  a binutils user has filed another PoC that exposes a problem in
  demangle_path_maybe_open_generics():

https://sourceware.org/bugzilla/show_bug.cgi?id=29312#c1

  I have redirected them to file a bug report with the gcc system, but in
  the hopes of getting a fix in quickly I am also attaching a patch
  here.  It just does the obvious thing of adding a recursion counter
  and limit to the function.

Cheers
  Nick

diff --git a/libiberty/rust-demangle.c b/libiberty/rust-demangle.c
index 36afcfae278..d6daf23af27 100644
--- a/libiberty/rust-demangle.c
+++ b/libiberty/rust-demangle.c
@@ -1082,6 +1082,18 @@ demangle_path_maybe_open_generics (struct rust_demangler *rdm)
   if (rdm->errored)
 return open;
 
+  if (rdm->recursion != RUST_NO_RECURSION_LIMIT)
+{
+  ++ rdm->recursion;
+  if (rdm->recursion > RUST_MAX_RECURSION_COUNT)
+	{
+	  /* FIXME: There ought to be a way to report
+	 that the recursion limit has been reached.  */
+	  rdm->errored = 1;
+	  goto end_of_func;
+	}
+}
+
   if (eat (rdm, 'B'))
 {
   backref = parse_integer_62 (rdm);
@@ -1107,6 +1119,11 @@ demangle_path_maybe_open_generics (struct rust_demangler *rdm)
 }
   else
 demangle_path (rdm, 0);
+
+ end_of_func:
+  if (rdm->recursion != RUST_NO_RECURSION_LIMIT)
+-- rdm->recursion;
+
   return open;
 }
 

Re: [PATCH] Pass PKG_CONFIG_PATH down from top-level Makefile

[Nick Clifton via Gcc-patches]

Hi Simon,


Ping.


Patch approved - please apply.

I appreciate that modifying these top level files is a bit nerve
wracking, but I think that you have done a good job in this case. :-)

Cheers
  Nick

Fix another Rust demangling bugs (PR 105039)

[Nick Clifton via Gcc-patches]

Hi Guys,

  Attached is a proposed patch to fix another Rust demangling bug
  reported in PR 105039.  I would like to say that this is the
  last time that we will see this problem for the Rust demangler,
  but I am not that naive...

  OK to apply ?

Cheers
  Nickdiff --git a/libiberty/rust-demangle.c b/libiberty/rust-demangle.c
index 3b24d63892a..d601182ac50 100644
--- a/libiberty/rust-demangle.c
+++ b/libiberty/rust-demangle.c
@@ -126,7 +126,7 @@ parse_integer_62 (struct rust_demangler *rdm)
 return 0;
 
   x = 0;
-  while (!eat (rdm, '_'))
+  while (!eat (rdm, '_') && !rdm->errored)
 {
   c = next (rdm);
   x *= 62;
@@ -1148,6 +1148,15 @@ demangle_const (struct rust_demangler *rdm)
   if (rdm->errored)
 return;
 
+  if (rdm->recursion != RUST_NO_RECURSION_LIMIT)
+{
+  ++ rdm->recursion;
+  if (rdm->recursion > RUST_MAX_RECURSION_COUNT)
+	/* FIXME: There ought to be a way to report
+	   that the recursion limit has been reached.  */
+	goto fail_return;
+}
+  
   if (eat (rdm, 'B'))
 {
   backref = parse_integer_62 (rdm);
@@ -1158,7 +1167,7 @@ demangle_const (struct rust_demangler *rdm)
   demangle_const (rdm);
   rdm->next = old_next;
 }
-  return;
+  goto pass_return;
 }
 
   ty_tag = next (rdm);
@@ -1167,7 +1176,7 @@ demangle_const (struct rust_demangler *rdm)
 /* Placeholder. */
 case 'p':
   PRINT ("_");
-  return;
+  goto pass_return;
 
 /* Unsigned integer types. */
 case 'h':
@@ -1200,18 +1209,20 @@ demangle_const (struct rust_demangler *rdm)
   break;
 
 default:
-  rdm->errored = 1;
-  return;
+  goto fail_return;
 }
 
-  if (rdm->errored)
-return;
-
-  if (rdm->verbose)
+  if (!rdm->errored && rdm->verbose)
 {
   PRINT (": ");
   PRINT (basic_type (ty_tag));
 }
+
+ fail_return:
+  rdm->errored = 1;
+ pass_return:
+  if (rdm->recursion != RUST_NO_RECURSION_LIMIT)
+-- rdm->recursion;
 }
 
 static void

RFA: libiberty: Fix infinite recursion in rust demangler (PRs 98886 and 99935)

[Nick Clifton via Gcc-patches]

Hi Guys,

  I would like to propose the patch below to fix a couple of sources
  of infinite recursion in libiberty's rust demangling code.  This patch
  is based upon the one submitted for PR 99935, but extended to cope
  with the case presented in PR 98886 and also fixed so that the "uint"
  type is not used.

  Tested with a patched version of the binutils sources on an
  x86-pc-linux-gnu target.

Cheers
  Nick

2022-01-26  Nick Clifton  

* rust-demangle.c (struct rust_demangler): Add a recursion
counter.
(demangle_path): Increment/decrement the recursion counter upon
entry and exit.  Fail if the counter exceeds a fixed limit.
(demangle_type): Likewise.
(rust_demangle_callback): Initialise the recursion counter,
disabling if requested by the option flags.

diff --git a/libiberty/rust-demangle.c b/libiberty/rust-demangle.c
index 18c760491bd..3b24d63892a 100644
--- a/libiberty/rust-demangle.c
+++ b/libiberty/rust-demangle.c
@@ -74,6 +74,12 @@ struct rust_demangler
   /* Rust mangling version, with legacy mangling being -1. */
   int version;
 
+  /* Recursion depth.  */
+  unsigned int recursion;
+  /* Maximum number of times demangle_path may be called recursively.  */
+#define RUST_MAX_RECURSION_COUNT  1024
+#define RUST_NO_RECURSION_LIMIT   ((unsigned int) -1)
+
   uint64_t bound_lifetime_depth;
 };
 
@@ -671,6 +677,15 @@ demangle_path (struct rust_demangler *rdm, int in_value)
   if (rdm->errored)
 return;
 
+  if (rdm->recursion != RUST_NO_RECURSION_LIMIT)
+{
+  ++ rdm->recursion;
+  if (rdm->recursion > RUST_MAX_RECURSION_COUNT)
+   /* FIXME: There ought to be a way to report
+  that the recursion limit has been reached.  */
+   goto fail_return;
+}
+
   switch (tag = next (rdm))
 {
 case 'C':
@@ -688,10 +703,7 @@ demangle_path (struct rust_demangler *rdm, int in_value)
 case 'N':
   ns = next (rdm);
   if (!ISLOWER (ns) && !ISUPPER (ns))
-{
-  rdm->errored = 1;
-  return;
-}
+   goto fail_return;
 
   demangle_path (rdm, in_value);
 
@@ -776,9 +788,15 @@ demangle_path (struct rust_demangler *rdm, int in_value)
 }
   break;
 default:
-  rdm->errored = 1;
-  return;
+  goto fail_return;
 }
+  goto pass_return;
+
+ fail_return:
+  rdm->errored = 1;
+ pass_return:
+  if (rdm->recursion != RUST_NO_RECURSION_LIMIT)
+-- rdm->recursion;
 }
 
 static void
@@ -870,6 +888,19 @@ demangle_type (struct rust_demangler *rdm)
   return;
 }
 
+   if (rdm->recursion != RUST_NO_RECURSION_LIMIT)
+{
+  ++ rdm->recursion;
+  if (rdm->recursion > RUST_MAX_RECURSION_COUNT)
+   /* FIXME: There ought to be a way to report
+  that the recursion limit has been reached.  */
+   {
+ rdm->errored = 1;
+ -- rdm->recursion;
+ return;
+   }
+}
+
   switch (tag)
 {
 case 'R':
@@ -1030,6 +1061,9 @@ demangle_type (struct rust_demangler *rdm)
   rdm->next--;
   demangle_path (rdm, 0);
 }
+
+  if (rdm->recursion != RUST_NO_RECURSION_LIMIT)
+-- rdm->recursion;
 }
 
 /* A trait in a trait object may have some "existential projections"
@@ -1320,6 +1354,7 @@ rust_demangle_callback (const char *mangled, int options,
   rdm.skipping_printing = 0;
   rdm.verbose = (options & DMGL_VERBOSE) != 0;
   rdm.version = 0;
+  rdm.recursion = (options & DMGL_NO_RECURSE_LIMIT) ? RUST_NO_RECURSION_LIMIT 
: 0;
   rdm.bound_lifetime_depth = 0;
 
   /* Rust symbols always start with _R (v0) or _ZN (legacy). */

RFA: Libiberty: Fix stack exhaunstion demangling corrupt rust names

[Nick Clifton via Gcc-patches]

Hi Guys,

  Attached is a proposed patch to fix PR 99935 and 100968, both
  of which are stack exhaustion problems in libiberty's Rust
  demangler.  The patch adds a recursion limit along the lines
  of the one already in place for the C++ demangler.

  OK to apply ?

Cheers
  Nick
diff --git a/libiberty/rust-demangle.c b/libiberty/rust-demangle.c
index 6fd8f6a4db0..df09b7b8fdd 100644
--- a/libiberty/rust-demangle.c
+++ b/libiberty/rust-demangle.c
@@ -74,6 +74,12 @@ struct rust_demangler
   /* Rust mangling version, with legacy mangling being -1. */
   int version;
 
+  /* Recursion depth.  */
+  uint recursion;
+  /* Maximum number of times demangle_path may be called recursively.  */
+#define RUST_MAX_RECURSION_COUNT  1024
+#define RUST_NO_RECURSION_LIMIT   ((uint) -1)
+
   uint64_t bound_lifetime_depth;
 };
 
@@ -671,6 +677,15 @@ demangle_path (struct rust_demangler *rdm, int in_value)
   if (rdm->errored)
 return;
 
+  if (rdm->recursion != RUST_NO_RECURSION_LIMIT)
+{
+  ++ rdm->recursion;
+  if (rdm->recursion > RUST_MAX_RECURSION_COUNT)
+	/* FIXME: There ought to be a way to report
+	   that the recursion limit has been reached.  */
+	goto fail_return;
+}
+
   switch (tag = next (rdm))
 {
 case 'C':
@@ -688,10 +703,7 @@ demangle_path (struct rust_demangler *rdm, int in_value)
 case 'N':
   ns = next (rdm);
   if (!ISLOWER (ns) && !ISUPPER (ns))
-{
-  rdm->errored = 1;
-  return;
-}
+	goto fail_return;
 
   demangle_path (rdm, in_value);
 
@@ -776,9 +788,15 @@ demangle_path (struct rust_demangler *rdm, int in_value)
 }
   break;
 default:
-  rdm->errored = 1;
-  return;
+  goto fail_return;
 }
+  goto pass_return;
+
+ fail_return:
+  rdm->errored = 1;
+ pass_return:
+  if (rdm->recursion != RUST_NO_RECURSION_LIMIT)
+-- rdm->recursion;
 }
 
 static void
@@ -1317,6 +1338,7 @@ rust_demangle_callback (const char *mangled, int options,
   rdm.skipping_printing = 0;
   rdm.verbose = (options & DMGL_VERBOSE) != 0;
   rdm.version = 0;
+  rdm.recursion = (options & DMGL_NO_RECURSE_LIMIT) ? RUST_NO_RECURSION_LIMIT : 0;
   rdm.bound_lifetime_depth = 0;
 
   /* Rust symbols always start with _R (v0) or _ZN (legacy). */

Re: [PATCH 2/4 REVIEW] libtool.m4: fix nm BSD flag detection

[Nick Clifton via Gcc-patches]

Hi Nick,


Ping?


Oops.


PR libctf/27482
* libtool.m4 (LT_PATH_NM): Try BSDization flags with a user-provided


Changes to libtool need to be posted to the libtool project:

  https://www.gnu.org/software/libtool/

They have mailing lists for bug reports and patch submissions.

Once the patch has been accepted there it can be backported to the gcc and
gdb/binutils repositories...

Cheers
  Nick

Re: Commit: Update libiberty sources

[Nick Clifton via Gcc-patches]

Hi H.J.


My patch is needed to build binutils with LTO.  I submitted a patch for GCC:

https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574405.html


Very well.  I have reappplied your patch to the mainline and 2.37 branch 
sources.

Cheers
  Nick

Re: RFC: Changing AC_PROG_CC to AC_PROG_CC_C99 in top level configure

[Nick Clifton via Gcc-patches]

Hi Guys,

On 4/30/21 7:36 PM, Simon Marchi wrote:

I think this fix is obvious enough, I encourage you to push it,


OK - I have pushed the patch to the mainline branches of both
the gcc and binutils-gfdb repositories.

Cheers
  Nick

Re: RFC: Changing AC_PROG_CC to AC_PROG_CC_C99 in top level configure

[Nick Clifton via Gcc-patches]

Hi Joseph,


This isn't an objection, since upgrading auto* for the toolchain can be
complicated, but note that AC_PROG_CC_C99 is obsolete in Autoconf 2.70 


Ah - in which case changing to an about-to-be-obsolete macro is probably 
a bad idea.



and instead AC_PROG_CC enables C11 mode if supported.  (So moving to the
latest Autoconf and Automake releases would supersede this change.)


Makes sense.  Is changing to autoconf 2.70 something that is planned for 
the near future ?


I actually have a Fedora BZ open suggesting that the binutils be 
upgraded to use autoconf 2.71.  I have put off doing this however as I 
am not an autoconf expert and I have no idea what the consequences might be.


Cheers
  Nick

RFC: Changing AC_PROG_CC to AC_PROG_CC_C99 in top level configure

[Nick Clifton via Gcc-patches]

Hi Guys,

  Given that gcc, gdb and now binutils are all now requiring C99 as a
  minimum version of C, are there any objections to updating
  configure.ac to reflect this ?

Cheers
  Nick

diff --git a/configure.ac b/configure.ac
index a721316d07b..59b4194fb24 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1278,7 +1278,7 @@ else
   WINDMC_FOR_BUILD="\$(WINDMC)"
 fi

-AC_PROG_CC
+AC_PROG_CC_C99
 AC_PROG_CXX

 # We must set the default linker to the linker used by gcc for the correct

Re: GCC: v850-elf

[Nick Clifton via Gcc-patches]

Hi JBG,


These three let it build.  One done.  Thanks for your support!


No worries.  Patch pushed.

Cheers
  Nick

Re: GCC: v850-elf

[Nick Clifton via Gcc-patches]

Hi Jan-Benedict,


However, next one is:



../.././gcc/defaults.h:938: error: "PREFERRED_DEBUGGING_TYPE" redefined 
[-Werror]
   938 | #define PREFERRED_DEBUGGING_TYPE NO_DEBUG


Ah - this is the same as the fix needed for the RX target.  Please try the 
attached
patch.  It includes my original patch, your addition to the patch and a fix for 
the
above problem.

Cheers
  Nick
diff --git a/gcc/config/v850/v850.c b/gcc/config/v850/v850.c
index 249cb400177..e0e5005d865 100644
--- a/gcc/config/v850/v850.c
+++ b/gcc/config/v850/v850.c
@@ -2181,7 +2181,7 @@ construct_restore_jr (rtx op)
   unsigned long int first;
   unsigned long int last;
   int i;
-  static char buff [100]; /* XXX */
+  static char buff [256]; /* XXX */
   
   if (count <= 2)
 {
@@ -2286,7 +2286,7 @@ construct_save_jarl (rtx op)
   unsigned long int first;
   unsigned long int last;
   int i;
-  static char buff [100]; /* XXX */
+  static char buff [255]; /* XXX */
   
   if (count <= (TARGET_LONG_CALLS ? 3 : 2)) 
 {
diff --git a/gcc/config/v850/v850.h b/gcc/config/v850/v850.h
index 23dfdf67dff..386f9f59e0b 100644
--- a/gcc/config/v850/v850.h
+++ b/gcc/config/v850/v850.h
@@ -700,6 +700,7 @@ typedef enum
 /* Use dwarf2 debugging info by default.  */
 #undef  PREFERRED_DEBUGGING_TYPE
 #define PREFERRED_DEBUGGING_TYPE   DWARF2_DEBUG
+#define DWARF2_DEBUGGING_INFO	   1
 
 #define DWARF2_FRAME_INFO  1
 #define DWARF2_UNWIND_INFO 0

RFA: libiberty: silence static analyzer warning

[Nick Clifton via Gcc-patches]

Hi Ian,

  One of the static analyzers we use is throwing up an error report for
  one of the libiberty source files:

Error: BUFFER_SIZE (CWE-474):
libiberty/sha1.c:261: overlapping_buffer: The source buffer ">buffer[16]" 
potentially overlaps with the destination buffer "ctx->buffer", which results 
in undefined behavior for "memcpy".
libiberty/sha1.c:261: remediation: Use memmove instead of "memcpy".
#  259|   sha1_process_block (ctx->buffer, 64, ctx);
#  260|   left_over -= 64;
#  261|-> memcpy (ctx->buffer, >buffer[16], left_over);
#  262| }
#  263| ctx->buflen = left_over;

  Looking at the source code I am not sure if the problem can actually
  be triggered in reality, but there seems to be no harm in being
  cautious, so I would like to ask for permission to apply the following
  patch:

diff --git a/libiberty/sha1.c b/libiberty/sha1.c
index e3d7f86e351..7d15d48d11d 100644
--- a/libiberty/sha1.c
+++ b/libiberty/sha1.c
@@ -258,7 +258,7 @@ sha1_process_bytes (const void *buffer, size_t len, struct 
sha1_ctx *ctx)
{
  sha1_process_block (ctx->buffer, 64, ctx);
  left_over -= 64;
- memcpy (ctx->buffer, >buffer[16], left_over);
+ memmove (ctx->buffer, >buffer[16], left_over);
}
   ctx->buflen = left_over;
 }

Cheers
  Nick
  

Re: GCC: v850-elf

[Nick Clifton via Gcc-patches]

Hi Jan-Benedict,


With my re-started testing efforts, I've got another one for you I
guess (`./configure --target=v850-elf && make all-gcc`):

../.././gcc/config/v850/v850.c: In function ‘char* construct_restore_jr(rtx)’:
../.././gcc/config/v850/v850.c:2260:22: error: ‘%s’ directive writing up to 39 
bytes into a region of size between 32 and 71 [-Werror=format-overflow=]
  2260 |   sprintf (buff, "movhi hi(%s), r0, r6\n\tmovea lo(%s), r6, r6\n\tjmp 
r6",
   |  
^~~~
  2261 | name, name);
   |   


I could not reproduce these in my local environment, but I suspect that
you are using a more recent version of gcc than me.  The fix looks obvious
however, so please could you try out the attached patch and let me know
if it resolves the problem ?

Cheers
  Nick
diff --git a/gcc/config/v850/v850.c b/gcc/config/v850/v850.c
index 249cb400177..db3002a2cfb 100644
--- a/gcc/config/v850/v850.c
+++ b/gcc/config/v850/v850.c
@@ -2181,7 +2181,7 @@ construct_restore_jr (rtx op)
   unsigned long int first;
   unsigned long int last;
   int i;
-  static char buff [100]; /* XXX */
+  static char buff [256]; /* XXX */
   
   if (count <= 2)
 {

[COMMITED]: rx.h: Define supported debugging types.

[Nick Clifton via Gcc-patches]

Hi Guys,

  I am applying the patch below to fix a problem building the rx port.
  The rx.h header file defines PREFERRED_DEBUGGING_TYPE but it was not
  defining the types of debugging it preferred.  This results in the
  definition in defaults.h being triggered and the compiler complaining
  about a redefinition.

Cheers
  Nick

gcc/ChangeLog
2021-03-09  Nick Clifton  

* config/rx/rx.h (DBX_DEBUGGING_INFO): Define.
(DWARF"_DEBUGGING_INFO): Define.

diff --git a/gcc/config/rx/rx.h b/gcc/config/rx/rx.h
index 8e23e311c03..59e1f7cfa96 100644
--- a/gcc/config/rx/rx.h
+++ b/gcc/config/rx/rx.h
@@ -628,6 +628,8 @@ typedef unsigned int CUMULATIVE_ARGS;
 #undef  PREFERRED_DEBUGGING_TYPE
 #define PREFERRED_DEBUGGING_TYPE (TARGET_AS100_SYNTAX \
  ? DBX_DEBUG : DWARF2_DEBUG)
+#define DBX_DEBUGGING_INFO 1
+#define DWARF2_DEBUGGING_INFO 1
 
 #define INCOMING_FRAME_SP_OFFSET   4
 #define ARG_POINTER_CFA_OFFSET(FNDECL) 4

Re: RFA: Remove use of register keyword in libiberty.h

[Nick Clifton via Gcc-patches]

Hi Guys,

>> include/ChangeLog
>> 2020-06-25  Nick Clifton  
>>
>>  * libiberty.h (bsearch_r): Remove use of the register keyword from
>>  the prototype.
>>
>> libiberty/ChangeLog
>> 2020-06-25  Nick Clifton  
>>
>>  * bsearch.c (bsearch): Remove use of register keyword.
>>  * bsearch_r.c (bsearch_r): Likewise.
> 
> Sure, this is fine.

Committed.

Cheers
  Nick

[COMMITTED} m32r: Disable movsicc pattern

[Nick Clifton via Gcc-patches]

Hi Guys,

  I am checking in the patch below to fix several failures in the GCC
  testsuite for the M32R target.  The issue is the movsicc pattern which
  is a holdover from when the port from converted from using cc0.  The
  pattern was written as if a previous instruction had set the CC bits,
  whereas in fact it is supposed to move CC bits into a general
  register.

  The patch disables the movsicc pattern, which means that some
  optimizations can be missed, but it does also fix the following
  tests:

> PASS: gcc.c-torture/execute/20040709-1.c   -O2  execution test
> PASS: gcc.c-torture/execute/20040709-1.c   -O3 -fomit-frame-pointer 
> -funroll-loops -fpeel-loops -ftracer -finline-functions  execution test
> PASS: gcc.c-torture/execute/20040709-1.c   -O3 -g  execution test
> PASS: gcc.c-torture/execute/20040709-1.c   -Os  execution test
> PASS: gcc.c-torture/execute/20040709-1.c   -O2 -flto -fno-use-linker-plugin 
> -flto-partition=none  execution test
> PASS: gcc.c-torture/execute/20040709-1.c   -O2 -flto -fuse-linker-plugin 
> -fno-fat-lto-objects  execution test
> PASS: gcc.c-torture/execute/20040709-2.c   -O2  execution test
> PASS: gcc.c-torture/execute/20040709-2.c   -O3 -fomit-frame-pointer 
> -funroll-loops -fpeel-loops -ftracer -finline-functions  execution test
> PASS: gcc.c-torture/execute/20040709-2.c   -O3 -g  execution test
> PASS: gcc.c-torture/execute/20040709-2.c   -Os  execution test
> PASS: gcc.c-torture/execute/20040709-2.c   -O2 -flto -fno-use-linker-plugin 
> -flto-partition=none  execution test
> PASS: gcc.c-torture/execute/20040709-2.c   -O2 -flto -fuse-linker-plugin 
> -fno-fat-lto-objects  execution test
> PASS: gcc.c-torture/execute/20040709-3.c   -O2  execution test
> PASS: gcc.c-torture/execute/20040709-3.c   -O3 -g  execution test
> PASS: gcc.c-torture/execute/20040709-3.c   -Os  execution test
> PASS: gcc.c-torture/execute/20040709-3.c   -O2 -flto -fno-use-linker-plugin 
> -flto-partition=none  execution test
> PASS: gcc.c-torture/execute/20040709-3.c   -O2 -flto -fuse-linker-plugin 
> -fno-fat-lto-objects  execution test
> PASS: gcc.dg/strcmpopt_2.c execution test
> PASS: gcc.dg/lto/pr67452 c_lto_pr67452_0.o-c_lto_pr67452_0.o link,  -O2 -flto 
> -fopenmp-simd 

Cheers
  Nick

gcc/ChangeLog
2020-06-25  Nick Clifton  

* config/m32r/m32r.md (movsicc): Disable pattern.

diff --git a/gcc/config/m32r/m32r.md b/gcc/config/m32r/m32r.md
index 823342af1b4..6ecd9ce89ab 100644
--- a/gcc/config/m32r/m32r.md
+++ b/gcc/config/m32r/m32r.md
@@ -2162,6 +2162,12 @@
   ""
   "
 {
+  /* FIXME: This expansion is hold over from a failed conversion of this
+ port away from using cc0.  It still relies upon the last comparison
+ being the one that is now tested.  Disabled for now in order to
+ improve the generation of working code.  */
+  FAIL;
+
   if (! zero_and_one (operands [2], operands [3]))
 FAIL;
 

Re: RFA: Remove use of register keyword in libiberty.h

[Nick Clifton via Gcc-patches]

Hi Nick, Hi Ian, 
>> In file included from gold/archive.cc:29:
>> include/libiberty.h:646:25: error: 'register' storage class
>>   specifier is deprecated and incompatible with C++17
>>   [-Werror,-Wdeprecated-register]
>>
>>   So I would like to apply the patch below to fix this.  Is this OK ?
> 
> Yes please! This was copied straight from bsearch.c, so you probably
> want to change bsearch.c and bsearch_r.c as well (just in case clang
> ever finds itself needing to build bsearch_r out of libiberty).

OK, here is a revised patch.  Ian - is this OK ?

Cheers
  Nick

include/ChangeLog
2020-06-25  Nick Clifton  

* libiberty.h (bsearch_r): Remove use of the register keyword from
the prototype.

libiberty/ChangeLog
2020-06-25  Nick Clifton  

* bsearch.c (bsearch): Remove use of register keyword.
* bsearch_r.c (bsearch_r): Likewise.
diff --git a/include/libiberty.h b/include/libiberty.h
index 0bb5b81d4ac..591e9ac48d4 100644
--- a/include/libiberty.h
+++ b/include/libiberty.h
@@ -643,9 +643,9 @@ extern int pwait (int, int *, int);
 
 /* Like bsearch, but takes and passes on an argument like qsort_r.  */
 
-extern void *bsearch_r (register const void *, const void *,
-			size_t, register size_t,
-			register int (*)(const void *, const void *, void *),
+extern void *bsearch_r (const void *, const void *,
+			size_t, size_t,
+			int (*)(const void *, const void *, void *),
 			void *);
 
 #if defined(HAVE_DECL_ASPRINTF) && !HAVE_DECL_ASPRINTF
diff --git a/libiberty/bsearch.c b/libiberty/bsearch.c
index 35fad19977c..18158b9591b 100644
--- a/libiberty/bsearch.c
+++ b/libiberty/bsearch.c
@@ -69,13 +69,13 @@ is respectively less than, matching, or greater than the array member.
  * look at item 3.
  */
 void *
-bsearch (register const void *key, const void *base0,
- size_t nmemb, register size_t size,
- register int (*compar)(const void *, const void *))
+bsearch (const void *key, const void *base0,
+ size_t nmemb, size_t size,
+ int (*compar)(const void *, const void *))
 {
-	register const char *base = (const char *) base0;
-	register int lim, cmp;
-	register const void *p;
+	const char *base = (const char *) base0;
+	int lim, cmp;
+	const void *p;
 
 	for (lim = nmemb; lim != 0; lim >>= 1) {
 		p = base + (lim >> 1) * size;
diff --git a/libiberty/bsearch_r.c b/libiberty/bsearch_r.c
index 79ebae9b0be..2a2ca6f5e23 100644
--- a/libiberty/bsearch_r.c
+++ b/libiberty/bsearch_r.c
@@ -70,14 +70,14 @@ is respectively less than, matching, or greater than the array member.
  * look at item 3.
  */
 void *
-bsearch_r (register const void *key, const void *base0,
-	   size_t nmemb, register size_t size,
-	   register int (*compar)(const void *, const void *, void *),
+bsearch_r (const void *key, const void *base0,
+	   size_t nmemb, size_t size,
+	   int (*compar)(const void *, const void *, void *),
 	   void *arg)
 {
-	register const char *base = (const char *) base0;
-	register int lim, cmp;
-	register const void *p;
+	const char *base = (const char *) base0;
+	int lim, cmp;
+	const void *p;
 
 	for (lim = nmemb; lim != 0; lim >>= 1) {
 		p = base + (lim >> 1) * size;

RFA: Remove use of register keyword in libiberty.h

[Nick Clifton via Gcc-patches]

Hi Ian, Hi Nick,

  Compiling the GOLD linker with Clang has started producing this error
  message:

In file included from gold/archive.cc:29:
include/libiberty.h:646:25: error: 'register' storage class
  specifier is deprecated and incompatible with C++17
  [-Werror,-Wdeprecated-register]

  So I would like to apply the patch below to fix this.  Is this OK ?
  
Cheers
  Nick

include/ChangeLog
2020-06-25  Nick Clifton  

* libiberty.h (bsearch_r): Remove use of the register keyword from
the prototype.

diff --git a/include/libiberty.h b/include/libiberty.h
index 0bb5b81d4a..591e9ac48d 100644
--- a/include/libiberty.h
+++ b/include/libiberty.h
@@ -643,9 +643,9 @@ extern int pwait (int, int *, int);
 
 /* Like bsearch, but takes and passes on an argument like qsort_r.  */
 
-extern void *bsearch_r (register const void *, const void *,
-   size_t, register size_t,
-   register int (*)(const void *, const void *, void *),
+extern void *bsearch_r (const void *, const void *,
+   size_t, size_t,
+   int (*)(const void *, const void *, void *),
void *);
 
 #if defined(HAVE_DECL_ASPRINTF) && !HAVE_DECL_ASPRINTF

 

RFA: Remove use of register keyword in libiberty.h

[Nick Clifton via Gcc-patches]

Hi Ian, Hi Nick,

  Comping the GOLD linker with Clang has started producing this error
  message:

In file included from gold/archive.cc:29:
include/libiberty.h:646:25: error: 'register' storage class
  specifier is deprecated and incompatible with C++17
  [-Werror,-Wdeprecated-register]

  So I would like to apply the patch below to fix this.  Is this OK ?
  
Cheers
  Nick

include/ChangeLog
2020-06-25  Nick Clifton  

* libiberty.h (bsearch_r): Remove use of the register keyword from
the prototype.

diff --git a/include/libiberty.h b/include/libiberty.h
index 0bb5b81d4a..591e9ac48d 100644
--- a/include/libiberty.h
+++ b/include/libiberty.h
@@ -643,9 +643,9 @@ extern int pwait (int, int *, int);
 
 /* Like bsearch, but takes and passes on an argument like qsort_r.  */
 
-extern void *bsearch_r (register const void *, const void *,
-   size_t, register size_t,
-   register int (*)(const void *, const void *, void *),
+extern void *bsearch_r (const void *, const void *,
+   size_t, size_t,
+   int (*)(const void *, const void *, void *),
void *);
 
 #if defined(HAVE_DECL_ASPRINTF) && !HAVE_DECL_ASPRINTF

 

RFA: Fix libiberty testsuite failure

[Nick Clifton]

Hi Ian,

  The libiberty testsuite in the gcc mainline is currently failing on
  the last test:

FAIL at line 1452, options :
in:  _Z3fooILPv0EEvPN9enable_ifIXeqT_LDnEEvE4typeE
out: void foo<(void*)0>(enable_if<((void*)0)==(decltype(nullptr)), 
void>::type*)
exp: void foo<(void*)0>(enable_if<((void*)0)==((decltype(nullptr))), 
void>::type*)

  To me it looks like the expected demangling is incorrect - it wants a
  double set of parentheses around decltype(nullptr) when I think that
  only one is needed.  So I would like to apply the patch below to fix
  this.

  Is this OK ?

Cheers
  Nick

libiberty/ChangeLog
2020-01-20  Nick Clifton  

* testsuite/demangle-expected: Fix expected demangling.

Index: libiberty/testsuite/demangle-expected
===
--- libiberty/testsuite/demangle-expected   (revision 280157)
+++ libiberty/testsuite/demangle-expected   (working copy)
@@ -1449,4 +1449,4 @@
 #PR91979 demangling nullptr expression
 
 _Z3fooILPv0EEvPN9enable_ifIXeqT_LDnEEvE4typeE
-void foo<(void*)0>(enable_if<((void*)0)==((decltype(nullptr))), void>::type*)
+void foo<(void*)0>(enable_if<((void*)0)==(decltype(nullptr)), void>::type*)

Re: [PATCH 0/2] Introduce a new GCC option, --record-gcc-command-line

[Nick Clifton]

Hi Egeyar,

Thanks for including me in this discussion.

>>> This option is similar to -frecord-gcc-switches.

For the record I will also note that there is -fverbose-asm which
does almost the same thing, but only records the options as comments
in the assembler.  They are never converted into data in the actual
object files.

It is also worth noting that if your goal is to record how a binary
was produced, possibly with an eye to reproducibility, then you may
also need to record some environment variables too.

One thing I found with annobin is that capturing preprocessor options
(eg -D_FORTIFY_SOURCE) can be quite hard from inside gcc, since often
they have already been processed and discarded.  I do not know if this
affects your actual patch though.

Speaking of annobin, I will bang the gcc plugin gong again here and say
that if your patch is rejected then you might want to consider turning
it into a plugin instead.  In that way you will not need approval from
the gcc maintainers.  But of course you will have to maintain and 
publicise the plugin yourself.

One other thought occurs to me, which is that if the patch is acceptable,
or at least the idea of it, then maybe it would be better to amalgamate
all of the current command line recording options into a single version.
Eg:

  --frecord-options=[dwarf,assembler,object]

where:

  --frecord-options=dwarf  is a synonym for -grecord-switches
  --frecord-options=assembler  is a synonym for -fverbose-asm
  --frecord-options=object is a synonym for your option

The user could supply one or more of the selectors to have the recording
happen in multiple places.

Just an idea.

Cheers
  Nick

Re: RFA: Synchronize top level files with binutils

[Nick Clifton]

Hi Richard,

  Please may I apply this patch to the gcc-9, gcc-8 and gcc-7 branches ?

  I have tested it on all three branches and found no problems.

Cheers
  Nick

2019-06-07  Nick Clifton  

Import these changes from the binutils/gdb repository:

2019-05-28  Nick Alcock  

* Makefile.def (dependencies): configure-libctf depends on all-bfd
and all its deps.
* Makefile.in: Regenerated.

2019-05-28  Nick Alcock  

* Makefile.def (host_modules): Add libctf.
* Makefile.def (dependencies): Likewise.
libctf depends on zlib, libiberty, and bfd.
* Makefile.in: Regenerated.
* configure.ac (host_libs): Add libctf.
* configure: Regenerated.
2019-06-07  Nick Clifton  

	Import these changes from the binutils/gdb repository:

	2019-05-28  Nick Alcock  

	* Makefile.def (dependencies): configure-libctf depends on all-bfd
	and all its deps.
	* Makefile.in: Regenerated.

	2019-05-28  Nick Alcock  

	* Makefile.def (host_modules): Add libctf.
	* Makefile.def (dependencies): Likewise.
	libctf depends on zlib, libiberty, and bfd.
	* Makefile.in: Regenerated.
	* configure.ac (host_libs): Add libctf.
	* configure: Regenerated.

Index: Makefile.def
===
--- Makefile.def	(revision 272111)
+++ Makefile.def	(working copy)
@@ -4,7 +4,7 @@
 // Makefile.in is generated from Makefile.tpl by 'autogen Makefile.def'.
 // This file was originally written by Nathanael Nerode.
 //
-//   Copyright 2002-2013 Free Software Foundation
+//   Copyright 2002-2019 Free Software Foundation
 //
 // This file is free software; you can redistribute it and/or modify
 // it under the terms of the GNU General Public License as published by
@@ -128,6 +128,8 @@
 		extra_make_flags='@extra_linker_plugin_flags@'; };
 host_modules= { module= libcc1; extra_configure_flags=--enable-shared; };
 host_modules= { module= gotools; };
+host_modules= { module= libctf; no_install=true; no_check=true;
+		bootstrap=true; };
 
 target_modules = { module= libstdc++-v3;
 		   bootstrap=true;
@@ -428,6 +430,7 @@
 dependencies = { module=all-binutils; on=all-build-bison; };
 dependencies = { module=all-binutils; on=all-intl; };
 dependencies = { module=all-binutils; on=all-gas; };
+dependencies = { module=all-binutils; on=all-libctf; };
 
 // We put install-opcodes before install-binutils because the installed
 // binutils might be on PATH, and they might need the shared opcodes
@@ -518,6 +521,14 @@
 dependencies = { module=all-fastjar; on=all-zlib; };
 dependencies = { module=all-fastjar; on=all-build-texinfo; };
 dependencies = { module=all-fastjar; on=all-libiberty; };
+dependencies = { module=all-libctf; on=all-libiberty; hard=true; };
+dependencies = { module=all-libctf; on=all-bfd; };
+dependencies = { module=all-libctf; on=all-zlib; };
+// So that checking for ELF support in BFD from libctf configure is possible.
+dependencies = { module=configure-libctf; on=all-bfd; };
+dependencies = { module=configure-libctf; on=all-intl; };
+dependencies = { module=configure-libctf; on=all-zlib; };
+dependencies = { module=configure-libctf; on=all-libiconv; };
 
 // Warning, these are not well tested.
 dependencies = { module=all-bison; on=all-intl; };
Index: configure.ac
===
--- configure.ac	(revision 272111)
+++ configure.ac	(working copy)
@@ -131,7 +131,7 @@
 
 # these libraries are used by various programs built for the host environment
 #f
-host_libs="intl libiberty opcodes bfd readline tcl tk itcl libgui zlib libbacktrace libcpp libdecnumber gmp mpfr mpc isl libelf libiconv"
+host_libs="intl libiberty opcodes bfd readline tcl tk itcl libgui zlib libbacktrace libcpp libdecnumber gmp mpfr mpc isl libelf libiconv libctf"
 
 # these tools are built for the host environment
 # Note, the powerpc-eabi build depends on sim occurring before gdb in order to

Re: RFA: Synchronize top level files with binutils

[Nick Clifton]

Hi Richard,

>>   OK, here is a resubmission of my patch with just the addition of the
>>   libctf patches this time.

  [Sorry - this one got put on a back burner].

> Would it be feasible to backport this to the other maintained branches
> so that the option of using them with current binutils would be available?

Do you have any particular branches in mind ?  There do seem to be quite a lot 
of them...

Cheers
  Nick

Re: RFA: Synchronize top level files with binutils

[Nick Clifton]

Hi Richard,

  OK, here is a resubmission of my patch with just the addition of the
  libctf patches this time.  (Sorry about the previous bad patch).
  Tested with a bootstrap and a normal build.  OK to apply ?

Cheers
  Nick

2019-06-07  Nick Clifton  

Import these changes from the binutils/gdb repository:

2019-05-28  Nick Alcock  

* Makefile.def (dependencies): configure-libctf depends on all-bfd
and all its deps.
* Makefile.in: Regenerated.

2019-05-28  Nick Alcock  

* Makefile.def (host_modules): Add libctf.
* Makefile.def (dependencies): Likewise.
libctf depends on zlib, libiberty, and bfd.
* Makefile.in: Regenerated.
* configure.ac (host_libs): Add libctf.
* configure: Regenerated.

Index: Makefile.def
===
--- Makefile.def(revision 272111)
+++ Makefile.def(working copy)
@@ -4,7 +4,7 @@
 // Makefile.in is generated from Makefile.tpl by 'autogen Makefile.def'.
 // This file was originally written by Nathanael Nerode.
 //
-//   Copyright 2002-2013 Free Software Foundation
+//   Copyright 2002-2019 Free Software Foundation
 //
 // This file is free software; you can redistribute it and/or modify
 // it under the terms of the GNU General Public License as published by
@@ -128,6 +128,8 @@
extra_make_flags='@extra_linker_plugin_flags@'; };
 host_modules= { module= libcc1; extra_configure_flags=--enable-shared; };
 host_modules= { module= gotools; };
+host_modules= { module= libctf; no_install=true; no_check=true;
+   bootstrap=true; };
 
 target_modules = { module= libstdc++-v3;
   bootstrap=true;
@@ -428,6 +430,7 @@
 dependencies = { module=all-binutils; on=all-build-bison; };
 dependencies = { module=all-binutils; on=all-intl; };
 dependencies = { module=all-binutils; on=all-gas; };
+dependencies = { module=all-binutils; on=all-libctf; };
 
 // We put install-opcodes before install-binutils because the installed
 // binutils might be on PATH, and they might need the shared opcodes
@@ -518,6 +521,14 @@
 dependencies = { module=all-fastjar; on=all-zlib; };
 dependencies = { module=all-fastjar; on=all-build-texinfo; };
 dependencies = { module=all-fastjar; on=all-libiberty; };
+dependencies = { module=all-libctf; on=all-libiberty; hard=true; };
+dependencies = { module=all-libctf; on=all-bfd; };
+dependencies = { module=all-libctf; on=all-zlib; };
+// So that checking for ELF support in BFD from libctf configure is possible.
+dependencies = { module=configure-libctf; on=all-bfd; };
+dependencies = { module=configure-libctf; on=all-intl; };
+dependencies = { module=configure-libctf; on=all-zlib; };
+dependencies = { module=configure-libctf; on=all-libiconv; };
 
 // Warning, these are not well tested.
 dependencies = { module=all-bison; on=all-intl; };
Index: configure.ac
===
--- configure.ac(revision 272111)
+++ configure.ac(working copy)
@@ -131,7 +131,7 @@
 
 # these libraries are used by various programs built for the host environment
 #f
-host_libs="intl libiberty opcodes bfd readline tcl tk itcl libgui zlib 
libbacktrace libcpp libdecnumber gmp mpfr mpc isl libelf libiconv"
+host_libs="intl libiberty opcodes bfd readline tcl tk itcl libgui zlib 
libbacktrace libcpp libdecnumber gmp mpfr mpc isl libelf libiconv libctf"
 
 # these tools are built for the host environment
 # Note, the powerpc-eabi build depends on sim occurring before gdb in order to

Re: RFA: Synchronize top level files with binutils

[Nick Clifton]

Hi Richard,

>>> +target_modules = { module= libmpx;
>>> +  bootstrap=true;
>>> +  lib_path=.libs; };
>>
>> It seems to re-introduce things that have been removed on the
>> GCC side.

> Is it just that one hunk that's problematic (I can't see any other
> non-relevant hunks)?  Without this patch, my unified tree builds are all
> broken and have been for a week now.

Actually I ave now reverted by libctf change to the binutils version
of Makefile.def, so I really need to resubmit this patch.  I will work
on it over the weekend and try again on Monday.

Cheers
  Nick

RFA: Synchronize top level files with binutils

[Nick Clifton]

Hi Guys,

  I would like to bring over a few additions that have recently been
  made to the binutils versions of the Makefile.def and configure.ac
  files.  Any objections ?

  Note - I did run a toolchain bootstrap after applying this patch
  locally and that went OK...

Cheers
  Nick

./ChangeLog
2019-05-29  Nick Clifton  

Import from binutils:
2019-05-29  Nick Clifton  

* configure.ac (noconfigdirs): Add libctf if the target does not use
the ELF file format.
* configure: Regenerate.

2019-05-28  Nick Alcock  

* Makefile.def (dependencies): configure-libctf depends on all-bfd
and all its deps.
* Makefile.in: Regenerated.

2019-05-28  Nick Alcock  

* Makefile.def (host_modules): Add libctf.
* Makefile.def (dependencies): Likewise.
libctf depends on zlib, libiberty, and bfd.
* Makefile.in: Regenerated.
* configure.ac (host_libs): Add libctf.
* configure: Regenerated.

Index: Makefile.def
===
--- Makefile.def(revision 271737)
+++ Makefile.def(working copy)
@@ -4,7 +4,7 @@
 // Makefile.in is generated from Makefile.tpl by 'autogen Makefile.def'.
 // This file was originally written by Nathanael Nerode.
 //
-//   Copyright 2002-2013 Free Software Foundation
+//   Copyright 2002-2019 Free Software Foundation
 //
 // This file is free software; you can redistribute it and/or modify
 // it under the terms of the GNU General Public License as published by
@@ -128,6 +128,8 @@
extra_make_flags='@extra_linker_plugin_flags@'; };
 host_modules= { module= libcc1; extra_configure_flags=--enable-shared; };
 host_modules= { module= gotools; };
+host_modules= { module= libctf; no_install=true; no_check=true;
+   bootstrap=true; };
 
 target_modules = { module= libstdc++-v3;
   bootstrap=true;
@@ -137,6 +139,9 @@
   bootstrap=true;
   lib_path=.libs;
   raw_cxx=true; };
+target_modules = { module= libmpx;
+  bootstrap=true;
+  lib_path=.libs; };
 target_modules = { module= libvtv;
   bootstrap=true;
   lib_path=.libs;
@@ -428,6 +433,7 @@
 dependencies = { module=all-binutils; on=all-build-bison; };
 dependencies = { module=all-binutils; on=all-intl; };
 dependencies = { module=all-binutils; on=all-gas; };
+dependencies = { module=all-binutils; on=all-libctf; };
 
 // We put install-opcodes before install-binutils because the installed
 // binutils might be on PATH, and they might need the shared opcodes
@@ -518,6 +524,14 @@
 dependencies = { module=all-fastjar; on=all-zlib; };
 dependencies = { module=all-fastjar; on=all-build-texinfo; };
 dependencies = { module=all-fastjar; on=all-libiberty; };
+dependencies = { module=all-libctf; on=all-libiberty; hard=true; };
+dependencies = { module=all-libctf; on=all-bfd; };
+dependencies = { module=all-libctf; on=all-zlib; };
+// So that checking for ELF support in BFD from libctf configure is possible.
+dependencies = { module=configure-libctf; on=all-bfd; };
+dependencies = { module=configure-libctf; on=all-intl; };
+dependencies = { module=configure-libctf; on=all-zlib; };
+dependencies = { module=configure-libctf; on=all-libiconv; };
 
 // Warning, these are not well tested.
 dependencies = { module=all-bison; on=all-intl; };
Index: configure.ac
===
--- configure.ac(revision 271737)
+++ configure.ac(working copy)
@@ -131,7 +131,7 @@
 
 # these libraries are used by various programs built for the host environment
 #f
-host_libs="intl libiberty opcodes bfd readline tcl tk itcl libgui zlib 
libbacktrace libcpp libdecnumber gmp mpfr mpc isl libelf libiconv"
+host_libs="intl libiberty opcodes bfd readline tcl tk itcl libgui zlib 
libbacktrace libcpp libdecnumber gmp mpfr mpc isl libelf libiconv libctf"
 
 # these tools are built for the host environment
 # Note, the powerpc-eabi build depends on sim occurring before gdb in order to
@@ -928,7 +934,23 @@
 ;;
 esac
 
+# Targets that do not use the ELF file format cannot support libctf.
 case "${target}" in
+  *-*-pe | *-*-*vms* | *-*-darwin | *-*-*coff* | *-*-wince | *-*-mingw*)
+noconfigdirs="$noconfigdirs libctf"
+;;
+  *-*-aout | *-*-osf* | *-*-go32 | *-*-macos* | *-*-rhapsody*)
+noconfigdirs="$noconfigdirs libctf"
+;;
+  *-*-netbsdpe | *-*-cygwin* | *-*-pep | *-*-msdos | *-*-winnt)
+noconfigdirs="$noconfigdirs libctf"
+;;
+  ns32k-*-* | pdp11-*-* | *-*-aix* | *-*-netbsdaout)
+noconfigdirs="$noconfigdirs libctf"
+;;
+esac
+
+case "${target}" in
   *-*-chorusos)
 ;;
   aarch64-*-darwin*)

RFA: Patch to fix severe recursion in d_count_templates_scopes (PR 89394)

[Nick Clifton]

Hi Ian,

  Attached is a proposed patch to fix PR 89394, which contains an
  artificial mangled name that triggers excessive recursion in
  d_count_templates_scopes.  The patch uses the same recursion limit
  that is already in place for d_print_comp, which I hope will be
  acceptable.

  There is one frag in the patch which is not directly related to this
  recursion problem however.  It extends the check in
  cplus_demangle_fill_name so that names with a negative length are
  rejected.  I had originally thought that the excessive recursion was
  due to a negative length string, although further investigation proved
  this guess to be wrong.  I felt that leaving the check in however
  would still be a good idea.

  Tested with no regressions with an x86_64-linux-gnu toolchain, as well
  as against the testcase in PR 89394.

  OK to apply ?

Cheers
  Nick

libiberty/ChangeLog
2019-03-21  Nick Clifton  

PR 89394
* cp-demangle.c (cplus_demangle_fill_name): Reject negative
lengths.
(d_count_templates_scopes): Replace num_templates and num_scopes
parameters with a struct d_print_info pointer parameter.  Adjust
body of the function accordingly.  Add recursion counter and check
that the recursion limit is not reached.
(d_print_init): Pass dpi parameter to d_count_templates_scopes.
Reset recursion counter afterwards, unless the recursion limit was
reached.

Index: libiberty/cp-demangle.c
===
--- libiberty/cp-demangle.c	(revision 269832)
+++ libiberty/cp-demangle.c	(working copy)
@@ -861,7 +861,7 @@
 int
 cplus_demangle_fill_name (struct demangle_component *p, const char *s, int len)
 {
-  if (p == NULL || s == NULL || len == 0)
+  if (p == NULL || s == NULL || len <= 0)
 return 0;
   p->d_printing = 0;
   p->type = DEMANGLE_COMPONENT_NAME;
@@ -4061,7 +4061,7 @@
are larger than the actual numbers encountered.  */
 
 static void
-d_count_templates_scopes (int *num_templates, int *num_scopes,
+d_count_templates_scopes (struct d_print_info *dpi,
 			  const struct demangle_component *dc)
 {
   if (dc == NULL)
@@ -4081,13 +4081,13 @@
   break;
 
 case DEMANGLE_COMPONENT_TEMPLATE:
-  (*num_templates)++;
+  dpi->num_copy_templates++;
   goto recurse_left_right;
 
 case DEMANGLE_COMPONENT_REFERENCE:
 case DEMANGLE_COMPONENT_RVALUE_REFERENCE:
   if (d_left (dc)->type == DEMANGLE_COMPONENT_TEMPLATE_PARAM)
-	(*num_scopes)++;
+	dpi->num_saved_scopes++;
   goto recurse_left_right;
 
 case DEMANGLE_COMPONENT_QUAL_NAME:
@@ -4152,42 +4152,42 @@
 case DEMANGLE_COMPONENT_TAGGED_NAME:
 case DEMANGLE_COMPONENT_CLONE:
 recurse_left_right:
-  d_count_templates_scopes (num_templates, num_scopes,
-d_left (dc));
-  d_count_templates_scopes (num_templates, num_scopes,
-d_right (dc));
+  /* PR 89394 - Check for too much recursion.  */
+  if (dpi->recursion > DEMANGLE_RECURSION_LIMIT)
+	/* FIXME: There ought to be a way to report to the
+	   user that the recursion limit has been reached.  */
+	return;
+
+  ++ dpi->recursion;
+  d_count_templates_scopes (dpi, d_left (dc));
+  d_count_templates_scopes (dpi, d_right (dc));
+  -- dpi->recursion;
   break;
 
 case DEMANGLE_COMPONENT_CTOR:
-  d_count_templates_scopes (num_templates, num_scopes,
-dc->u.s_ctor.name);
+  d_count_templates_scopes (dpi, dc->u.s_ctor.name);
   break;
 
 case DEMANGLE_COMPONENT_DTOR:
-  d_count_templates_scopes (num_templates, num_scopes,
-dc->u.s_dtor.name);
+  d_count_templates_scopes (dpi, dc->u.s_dtor.name);
   break;
 
 case DEMANGLE_COMPONENT_EXTENDED_OPERATOR:
-  d_count_templates_scopes (num_templates, num_scopes,
-dc->u.s_extended_operator.name);
+  d_count_templates_scopes (dpi, dc->u.s_extended_operator.name);
   break;
 
 case DEMANGLE_COMPONENT_FIXED_TYPE:
-  d_count_templates_scopes (num_templates, num_scopes,
-dc->u.s_fixed.length);
+  d_count_templates_scopes (dpi, dc->u.s_fixed.length);
   break;
 
 case DEMANGLE_COMPONENT_GLOBAL_CONSTRUCTORS:
 case DEMANGLE_COMPONENT_GLOBAL_DESTRUCTORS:
-  d_count_templates_scopes (num_templates, num_scopes,
-d_left (dc));
+  d_count_templates_scopes (dpi, d_left (dc));
   break;
 
 case DEMANGLE_COMPONENT_LAMBDA:
 case DEMANGLE_COMPONENT_DEFAULT_ARG:
-  d_count_templates_scopes (num_templates, num_scopes,
-dc->u.s_unary_num.sub);
+  d_count_templates_scopes (dpi, dc->u.s_unary_num.sub);
   break;
 }
 }
@@ -4222,8 +4222,12 @@
   dpi->next_copy_template = 0;
   dpi->num_copy_templates = 0;
 
-  d_count_templates_scopes (>num_copy_templates,
-			>num_saved_scopes, dc);
+  d_count_templates_scopes (dpi, dc);
+  /* If we did not reach the rec

Re: RFA: libiberty: Add a limit on demangling qualifiers (PR 87241)

[Nick Clifton]

Hi Jason,

> This issue also will be resolved by disabling or removing the old
> demangling code, which I haven't seen anyone argue against.

Doh - of course.  I withdraw my patch and I hope that yours will go in soon.

Cheers
  Nick

Re: RFA: libiberty: Add a limit on demangling qualifiers (PR 87241) (version 2)

[Nick Clifton]

Hi Ian,

> I thought we were removing the old demangling schemes?

Doh!  yes, I totally forgot.  So I will withdraw this patch in favour of 
Jason's.

Cheers
  Nick

RFA: libiberty: Add a limit on demangling qualifiers (PR 87241) (version 2)

[Nick Clifton]

Hi Ian,

  *sigh* 5 minutes after sending the patch for this PR, I realised that
   I had made a mistake.  I should have conditionalized the limit on the
   number of supported qualifiers, so that the check is only made if we
   have resource limits enabled.  Like this:

Cheers
  Nick

Index: libiberty/cplus-dem.c
===
--- libiberty/cplus-dem.c   (revision 267043)
+++ libiberty/cplus-dem.c   (working copy)
@@ -3443,6 +3443,20 @@
   success = 0;
 }
 
+  if ((work->options & DMGL_NO_RECURSE_LIMIT) == 0)
+{
+  /* PR 87241: Catch malicious input that will try to trick this code into
+allocating a ridiculous amount of memory via the remember_Ktype()
+function.
+The choice of DEMANGLE_RECURSION_LIMIT is somewhat arbitrary.  Possibly
+a better solution would be to track how much memory remember_Ktype
+allocates and abort when some upper limit is reached.  */
+  if (qualifiers > DEMANGLE_RECURSION_LIMIT)
+   /* FIXME: We ought to have some way to tell the user that
+  this limit has been reached.  */
+   success = 0;
+}
+
   if (!success)
 return success;
 

RFA: libiberty: Add a limit on demangling qualifiers (PR 87241)

[Nick Clifton]

Hi Ian,

  Sorry to bother you, but I have another libiberty demangler resource
  exhaustion prevention patch to present.  This one is for:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87241

  Jonathan Wakely reported that __cxa_demanlge() was returning a -2
  result, but I did not see this.  Instead I found that
  consume_count_with_underscores() is returning a very large number
  (because a very large value is encoded in the mangled string) and this
  is resulting in many calls to remember_Ktype() which eventually
  exhaust the amount of memory available.

  The attached patch is a simplistic approach to solving this problem by
  adding a hard upper limit on the number of qualifiers that will be
  allowed by the demangler.  I am not sure if this is the best approach
  to solving the problem, but it is a simple one, and I would think one
  that would not prevent the demangling of any real mangled names.  The
  limit does not have to be DEMANGLE_RECURSE_LIMIT of course.  I just
  chose that value because it was convenient and of a size that I
  thought was appropriate.

  I also did run the libiberty testsuite this time, with no failures
  reported. :-)

  OK to apply ?

Cheers
  Nick

libiberty/ChangeLog
2018-12-12  Nick Clifton  

* cplus-dem.c (demangle_qualified): Add an upper limit on the
number of qualifiers supported, based upon the value of
DEMANGLE_RECURSE_LIMIT.

Index: libiberty/cplus-dem.c
===
--- libiberty/cplus-dem.c   (revision 267043)
+++ libiberty/cplus-dem.c   (working copy)
@@ -3443,6 +3443,17 @@
   success = 0;
 }
 
+  /* PR 87241: Catch malicious input that will try to trick this code into
+ allocating a ridiculous amount of memory via the remember_Ktype()
+ function.
+ The choice of DEMANGLE_RECURSION_LIMIT is somewhat arbitrary.  Possibly
+ a better solution would be to track how much memory remember_Ktype
+ allocates and abort when some upper limit is reached.  */
+  if (qualifiers > DEMANGLE_RECURSION_LIMIT)
+/* FIXME: We ought to have some way to tell the user that
+   this limit has been reached.  */
+success = 0;
+
   if (!success)
 return success;
 

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Nick Clifton]

Hi David,

> Apologies in advance if this has been covered, as I've only been half-
> watching this thread, but is it always the case that the recursion
> depth can be bounded by some scalar multiple of the number of
> characters in the name?

Probably, but the point of this patch is to add a fixed limit that
prevents too much recursion from being performed.  The CVEs that I
have been trying to fix have been using mangled names with 20K-30K
characters in them, so creating a recursion limit based on the 
length of the input would not prevent the stack exhaustion. :-(

My hope is that we can choose a value that will allow any realistic
mangled name to be decoded, but which will prevent these fuzzers from
generating arbitrary length strings which exhaust the machines resources.

Cheers
  Nick

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Nick Clifton]

Hi Jakub,

>> My current suggestion
>> is to raise the limit to 2048, which allows the libiberty patch to 
>> pass.  But do you have a feel for how much is a realistic limit ?
> 
> For recursion limit I think that is fine.
> For just stack size limit, I think it is extremely small.
> I see that in the function it allocates on 64-bit 24 bytes times
> num_comps using alloca, so 48 bytes per character in the mangled name,
> and a pointer for each character in the mangled name.
> That is 112KB per 2048 bytes long mangled name.
> 
> Dunno how much stack can we expect to be usable.

Currently the patched libiberty uses the DEMANGLE_RECURSION_LIMIT value
in two ways.  The first is as a limit on the number of levels of recursion
of specific functions inside the demangler.  The second is as a check on
the number of component structures that will be allocated on the stack.
(See cp-demangle.c:d_demangle_callback).  One of the CVEs that I was checking
was triggering stack exhaustion this way, which is why I added the check.

There is at least one other function where a similar stack allocation
happens (cplus_demangle_print_callback) but I was not sure if this could
be triggered with the other limits in place, and I did not have a reproducer
that touched it, so I left it alone.

Cheers
  Nick

Re: [PATCH] Set DEMANGLE_RECURSION_LIMIT to 1536

[Nick Clifton]

Hi Michael,

> I think this points toward the limit being _much_ too low.

Fair enough - several other people have said this as well.  So
I have proposed an alternative patch instead.  My current suggestion
is to raise the limit to 2048, which allows the libiberty patch to 
pass.  But do you have a feel for how much is a realistic limit ?


> demangler, just looking at the symbol name and demangled result I don't 
> readily see where the depth of recursion really is more than 1024, are 
> there perhaps some recursion_level-- statements skipped?

I do not think so.  Unless there are some long jumps in the demangling code ?
I did a quick scan and did not find any, but I could have missed something.
Plus of course I cannot guarantee that my patch is bug free, although looking
at it again I do not see where I missed a level decrement.  

I think that the demangling code just is really recursive...

Cheers
  Nick

Re: RFC: libiberty PATCH to disable demangling of ancient mangling schemes

[Nick Clifton]

Hi Guys,

 Looks good to me.  Independently, do you see a reason not to disable the
 old demangler entirely?

>>   * How likely is it that there are old toolchain in use out there that 
>> still 
>> use the v2 mangling ?

> GCC 3.0 and up used the new (Itanium C++ ABI) mangling, 2.95 and older used 
> the old
> mangling (2.96-RH used the new mangling I believe).
> So you need compiler older than 17.5 years to have the old mangling.
> Such a compiler didn't support most of the contemporarily used platforms
> though at all (e.g. x86-64, powerpc64le, aarch64, I believe not even
> powerpc64-linux).

Well that is good enough for me. :-)  I do not have the power to approve
the patch, but I would certainly be happy to see it go in.

Cheers
  Nick

Re: RFC: libiberty PATCH to disable demangling of ancient mangling schemes

[Nick Clifton]

Hi Jason,

>> Looks good to me.  Independently, do you see a reason not to disable the
>> old demangler entirely?
> 
> Like so.  Does anyone object to this?  These mangling schemes haven't
> been relevant in decades.

I am not really familiar with this old scheme, so please excuse my ignorance
in asking these questions:

  * How likely is it that there are old toolchain in use out there that still 
use the v2 mangling ?  Ie I guess that I am asking "which generation(s)
of gcc used v2 mangling ?"

  * If a user does try to demangle a v2 mangled name, what will happen ?
Ie I guess I am asking if they will be given a hint that they need to use
an older toolchain in order to demangle the names.

Cheers
  Nick

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]

[Nick Clifton]

Hi Ian,

  Is the patch OK with you ?

Cheers
  Nick

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v5]

[Nick Clifton]

Hi Pedro,

> The issue pointed out by
> 
>  https://gcc.gnu.org/ml/gcc-patches/2018-11/msg02592.html
> 
> is still present in this version.

Doh!  Yes I meant to fix that one too, but forgot.

> Also, noticed a typo here:
> 
>> +/* If DMGL_NO_RECURE_LIMIT is not enabled, then this is the value used as
> 
> Typo: "RECURE"

Oops - thanks.

OK, revised (v5) patch attached.  Is this version acceptable to all ?

Cheers
  Nick

Index: include/demangle.h
===
--- include/demangle.h	(revision 266771)
+++ include/demangle.h	(working copy)
@@ -68,6 +68,17 @@
 /* If none of these are set, use 'current_demangling_style' as the default. */
 #define DMGL_STYLE_MASK (DMGL_AUTO|DMGL_GNU|DMGL_LUCID|DMGL_ARM|DMGL_HP|DMGL_EDG|DMGL_GNU_V3|DMGL_JAVA|DMGL_GNAT|DMGL_DLANG|DMGL_RUST)
 
+/* Disable a limit on the depth of recursion in mangled strings.
+   Note if this limit is disabled then stack exhaustion is possible when
+   demangling pathologically complicated strings.  Bug reports about stack
+   exhaustion when the option is enabled will be rejected.  */  
+#define DMGL_NO_RECURSE_LIMIT (1 << 18)	
+
+/* If DMGL_NO_RECURSE_LIMIT is not enabled, then this is the value used as
+   the maximum depth of recursion allowed.  It should be enough for any
+   real-world mangled name.  */
+#define DEMANGLE_RECURSION_LIMIT 1024
+  
 /* Enumeration of possible demangling styles.
 
Lucid and ARM styles are still kept logically distinct, even though
Index: libiberty/cp-demangle.c
===
--- libiberty/cp-demangle.c	(revision 266771)
+++ libiberty/cp-demangle.c	(working copy)
@@ -2852,21 +2852,35 @@
 static struct demangle_component *
 d_function_type (struct d_info *di)
 {
-  struct demangle_component *ret;
+  struct demangle_component *ret = NULL;
 
-  if (! d_check_char (di, 'F'))
-return NULL;
-  if (d_peek_char (di) == 'Y')
+  if ((di->options & DMGL_NO_RECURSE_LIMIT) == 0)
 {
-  /* Function has C linkage.  We don't print this information.
-	 FIXME: We should print it in verbose mode.  */
-  d_advance (di, 1);
+  if (di->recursion_level > DEMANGLE_RECURSION_LIMIT)
+	/* FIXME: There ought to be a way to report
+	   that the recursion limit has been reached.  */
+	return NULL;
+
+  di->recursion_level ++;
 }
-  ret = d_bare_function_type (di, 1);
-  ret = d_ref_qualifier (di, ret);
 
-  if (! d_check_char (di, 'E'))
-return NULL;
+  if (d_check_char (di, 'F'))
+{
+  if (d_peek_char (di) == 'Y')
+	{
+	  /* Function has C linkage.  We don't print this information.
+	 FIXME: We should print it in verbose mode.  */
+	  d_advance (di, 1);
+	}
+  ret = d_bare_function_type (di, 1);
+  ret = d_ref_qualifier (di, ret);
+  
+  if (! d_check_char (di, 'E'))
+	ret = NULL;
+}
+
+  if ((di->options & DMGL_NO_RECURSE_LIMIT) == 0)
+di->recursion_level --;
   return ret;
 }
 
@@ -6203,6 +6217,7 @@
   di->expansion = 0;
   di->is_expression = 0;
   di->is_conversion = 0;
+  di->recursion_level = 0;
 }
 
 /* Internal implementation for the demangler.  If MANGLED is a g++ v3 ABI
@@ -6242,6 +6257,20 @@
 
   cplus_demangle_init_info (mangled, options, strlen (mangled), );
 
+  /* PR 87675 - Check for a mangled string that is so long
+ that we do not have enough stack space to demangle it.  */
+  if (((options & DMGL_NO_RECURSE_LIMIT) == 0)
+  /* This check is a bit arbitrary, since what we really want to do is to
+	 compare the sizes of the di.comps and di.subs arrays against the
+	 amount of stack space remaining.  But there is no portable way to do
+	 this, so instead we use the recursion limit as a guide to the maximum
+	 size of the arrays.  */
+  && (unsigned long) di.num_comps > DEMANGLE_RECURSION_LIMIT)
+{
+  /* FIXME: We need a way to indicate that a stack limit has been reached.  */
+  return 0;
+}
+
   {
 #ifdef CP_DYNAMIC_ARRAYS
 __extension__ struct demangle_component comps[di.num_comps];
Index: libiberty/cp-demangle.h
===
--- libiberty/cp-demangle.h	(revision 266771)
+++ libiberty/cp-demangle.h	(working copy)
@@ -122,6 +122,9 @@
   /* Non-zero if we are parsing the type operand of a conversion
  operator, but not when in an expression.  */
   int is_conversion;
+  /* If DMGL_NO_RECURSE_LIMIT is not active then this is set to
+ the current recursion level.  */
+  unsigned int recursion_level;
 };
 
 /* To avoid running past the ending '\0', don't:
Index: libiberty/cplus-dem.c
===
--- libiberty/cplus-dem.c	(revision 266771)
+++ libiberty/cplus-dem.c	(working copy)
@@ -146,6 +146,7 @@
   int *proctypevec; /* Indices of currently processed remembered typevecs.  */
   int proctypevec_size;
   int nproctypes;
+  unsigned int recursion_level;
 };
 
 #define PRINT_ANSI_QUALIFIERS (work -> 

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v4]

[Nick Clifton]

Hi Ian,

>>> Shouldn't we make it fool-proof by instead introducing a 
>>> DMGL_NO_RECURSION_LIMIT

> You don't need my blessing--I wrote that code ages ago--but I agree
> with Richard that in practice it's OK to limit recursion depth by
> default.  Real symbols have very limited recursion requirements.

OK then, here is a fourth revision of the patch.

In this version:

  * The demangler option has been renamed to DMGHL_NO_RECURSE_LIMIT
and if the option is not present then the limit is enforced.

  * I also found another PR that is fixed by the patch, although I had
to make sure that the affected code could handle NULL pointers 
properly afterwards.

  OK to apply ?

Cheers
  Nick


include/ChangeLog
2018-11-29  Nick Clifton  

* demangle.h (DMGL_NO_RECURSE_LIMIT): Define.
(DEMANGLE_RECURSION_LIMIT): Define

libiberty/ChangeLog
2018-11-29  Nick Clifton  

PR 87681
PR 87675
PR 87636
PR 87350
PR 87335
* cp-demangle.h (struct d_info): Add recursion_level field.
* cp-demangle.c (d_function_type): Add recursion counter.
If the recursion limit is reached and the check is not disabled,
then return with a failure result.
(cplus_demangle_init_info): Initialise the recursion_level field.
(d_demangle_callback): If the recursion limit is enabled, check
for a mangled string that is so long that there is not enough
stack space for the local arrays.
* cplus-dem.c (struct work): Add recursion_level field.
(squangle_mop_up): Set the numb and numk fields to zero.
(work_stuff_copy_to_from): Handle the case where a btypevec or 
ktypevec field is NULL.
(demangle_nested_args): Add recursion counter.  If
the recursion limit is not disabled and reached, return with a
failure result.
Index: include/demangle.h
===
--- include/demangle.h	(revision 266771)
+++ include/demangle.h	(working copy)
@@ -68,6 +68,17 @@
 /* If none of these are set, use 'current_demangling_style' as the default. */
 #define DMGL_STYLE_MASK (DMGL_AUTO|DMGL_GNU|DMGL_LUCID|DMGL_ARM|DMGL_HP|DMGL_EDG|DMGL_GNU_V3|DMGL_JAVA|DMGL_GNAT|DMGL_DLANG|DMGL_RUST)
 
+/* Disable a limit on the depth of recursion in mangled strings.
+   Note if this limit is disabled then stack exhaustion is possible when
+   demangling pathologically complicated strings.  Bug reports about stack
+   exhaustion when the option is enabled will be rejected.  */  
+#define DMGL_NO_RECURSE_LIMIT (1 << 18)	
+
+/* If DMGL_NO_RECURE_LIMIT is not enabled, then this is the value used as
+   the maximum depth of recursion allowed.  It should be enough for any
+   real-world mangled name.  */
+#define DEMANGLE_RECURSION_LIMIT 1024
+  
 /* Enumeration of possible demangling styles.
 
Lucid and ARM styles are still kept logically distinct, even though
Index: libiberty/cp-demangle.c
===
--- libiberty/cp-demangle.c	(revision 266771)
+++ libiberty/cp-demangle.c	(working copy)
@@ -2852,21 +2852,35 @@
 static struct demangle_component *
 d_function_type (struct d_info *di)
 {
-  struct demangle_component *ret;
+  struct demangle_component *ret = NULL;
 
-  if (! d_check_char (di, 'F'))
-return NULL;
-  if (d_peek_char (di) == 'Y')
+  if ((di->options & DMGL_NO_RECURSE_LIMIT) == 0)
 {
-  /* Function has C linkage.  We don't print this information.
-	 FIXME: We should print it in verbose mode.  */
-  d_advance (di, 1);
+  if (di->recursion_level > DEMANGLE_RECURSION_LIMIT)
+	/* FIXME: There ought to be a way to report
+	   that the recursion limit has been reached.  */
+	return NULL;
+
+  di->recursion_level ++;
 }
-  ret = d_bare_function_type (di, 1);
-  ret = d_ref_qualifier (di, ret);
 
-  if (! d_check_char (di, 'E'))
-return NULL;
+  if (d_check_char (di, 'F'))
+{
+  if (d_peek_char (di) == 'Y')
+	{
+	  /* Function has C linkage.  We don't print this information.
+	 FIXME: We should print it in verbose mode.  */
+	  d_advance (di, 1);
+	}
+  ret = d_bare_function_type (di, 1);
+  ret = d_ref_qualifier (di, ret);
+  
+  if (! d_check_char (di, 'E'))
+	ret = NULL;
+}
+
+  if ((di->options & DMGL_NO_RECURSE_LIMIT) == 0)
+di->recursion_level --;
   return ret;
 }
 
@@ -6203,6 +6217,7 @@
   di->expansion = 0;
   di->is_expression = 0;
   di->is_conversion = 0;
+  di->recursion_level = 0;
 }
 
 /* Internal implementation for the demangler.  If MANGLED is a g++ v3 ABI
@@ -6242,6 +6257,20 @@
 
   cplus_demangle_init_info (mangled, options, strlen (mangled), );
 
+  /* PR 87675 - Check for a mangled string that is so long
+ that we do not have enough stack space to demangle it.  */
+  if (((options & DMGL_NO_RECURSE_LIMIT) == 0)
+  /* This check is 

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler

[Nick Clifton]

Hi Cary,

> In order to handle arbitrary user input without crashing, perhaps the
> demangler should switch from recursive descent parsing to a state
> machine, where exhaustion of resources can be handled gracefully.

I think that that would be a better long term fix for the problem,
but it is not one that I have time to work on right now.

My main goal with this patch submission is to stop the flood of PR 
and CVEs about mangled inputs that trigger stack exhaustion.  Being 
able to properly demangle such inputs would be nice, but not something
that I think should be a priority.  I think that in real life no 
program is ever going to generate a mangled name that is sufficiently 
complex to trigger a seg-fault this way, so the only real purpose of
the patch is to resolve these PRs and stop more from being filed.

Cheers
  Nick

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v3]

[Nick Clifton]

Hi Richard,

>>   * The description of the DMGL_RECURSE_LIMIT option in demangle.h has
>> been enhanced to add a note that if the option is not used, then
>> bug reports about stack overflows in the demangler will be rejected.
> 
> Shouldn't we make it fool-proof by instead introducing a 
> DMGL_NO_RECURSION_LIMIT
> flag and when not set default to limiting recursion?

Well I wanted the patch to be backwards compatible. Just on the
general principle of not surprising users/programmers by changing 
things without telling them first.

I could change this of course, but I would rather have Ian's blessing
first.

Cheers
  Nick

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler [v3]

[Nick Clifton]

Hi Guys,

>> I think it would be fine to have a large fixed limit plus a flag to
>> disable the limit.

Great - in which case please may I present version 3 of the patch.  In 
this version:

  * The cplus_demangle_set_recursion_limit() function has been removed
and instead a new constant - DEMANGLE_RECURSION_LIMIT - is defined in
demangle.h.

  * The recursion counters in cp-demangle.c have been merged into one
counter, now contained in the d_info structure.

  * In cplus-dem.c the recursion counter has been moved into the work
structure.

  * The description of the DMGL_RECURSE_LIMIT option in demangle.h has
been enhanced to add a note that if the option is not used, then
bug reports about stack overflows in the demangler will be rejected.

  * The binutils patch has been updated to reflect these changes.  The
addr2line, cxxfilt, nm and objdump programs now have two new options
--recurse-limit and --no-recurse-limit, with --recurse-limit being
the default.  The documentation is updated to describe these options
and to also add a note about bug reports being rejected if 
--no-recurse-limit is used.

What do you think, is this version acceptable ?

Cheers
  Nick

libiberty/ChangeLog
2018-11-29  Nick Clifton  

PR 87681
PR 87675
PR 87636
PR 87335
* cp-demangle.h (struct d_info): Add recursion_limit field.
* cp-demangle.c (d_function_type): If the recursion limit is 
enabled and reached, return with a failure result.
(d_demangle_callback): If the recursion limit is enabled, check
for a mangled string that is so long that there is not enough
stack space for the local arrays.
* cplus-dem.c (struct work): Add recursion_level field.
(demangle_nested_args): If the recursion limit is enabled and 
reached, return with a failure result.

include/ChangeLog
2018-11-29  Nick Clifton  

* demangle.h (DMGL_RECURSE_LIMIT): Define.
(DEMANGLE_RECURSION_LIMIT): Prototype.

binutils/ChangeLog
2018-11-29  Nick Clifton  

* addr2line.c (demangle_flags): New static variable.
(long_options): Add --recurse-limit and --no-recurse-limit.
(translate_address): Pass demangle_flags to bfd_demangle.
(main): Handle --recurse-limit and --no-recurse-limit options.
* cxxfilt.c (flags): Add DMGL_RECURSE_LIMIT.
(long_options): Add --recurse-limit and --no-recurse-limit.
(main): Handle new options.
* dlltool.c (gen_def_file): Include DMGL_RECURSE_LIMIT in flags
passed to cplus_demangle.
* nm.c (demangle_flags): New static variable.
(long_options): Add --recurse-limit and --no-recurse-limit.
(main): Handle new options.
* objdump.c (demangle_flags): New static variable.
(usage): Add --recurse-limit and --no-recurse-limit.
(long_options): Likewise.
(objdump_print_symname): Pass demangle_flags to bfd_demangle.
(disassemble_section): Likewise.
(dump_dymbols): Likewise.
(main): Handle new options.
* prdbg.c (demangle_flags): New static variable.
(tg_variable): Pass demangle_flags to demangler.
(tg_start_function): Likewise.
* stabs.c (demangle_flags): New static variable.
(stab_demangle_template): Pass demangle_flags to demangler.
(stab_demangle_v3_argtypes): Likewise.
(stab_demangle_v3_arg): Likewise.
* doc/binutuls.texi: Document new command line options.
* NEWS: Mention the new feature.
* testsuite/config/default.exp (CXXFILT): Define if not already
defined.
(CXXFILTFLAGS): Likewise.
* testsuite/binutils-all/cxxfilt.exp: New file.  Runs a few
simple tests of the cxxfilt program.
diff --git a/binutils/NEWS b/binutils/NEWS
index a3ee86ef7f..5ed61c8513 100644
--- a/binutils/NEWS
+++ b/binutils/NEWS
@@ -1,5 +1,16 @@
 -*- text -*-
 
+* The addr2line, c++filt, nm and objdump tools now have a limit on the
+  maximum amount of recursion that is allowed whilst demangling strings.
+  The value for this limit is defined by the DEMANGLE_RECRUSE_LIMIT
+  constant declared in the include/demangle.h header file.  At the time
+  of writing this constant has the value of 1024.
+
+  The --no-recurse-limit option can be used to remove the limit, restoring
+  the behaviour of earlier versions of these tools.  This may be needed in
+  order to dmangle truely complicated names, but it also leaves the tools
+  vulnerable to stack exhaustion from maliciously constructed mangled names.
+
 * Objdump's --disassemble option can now take a parameter, specifying the
   starting symbol for disassembly.  Disassembly will continue from this
   symbol up to the next symbol.
diff --git a/binutils/addr2line.c b/binutils/addr2line.c
index 008e62026e..3085806a4a 100644
--- a/binutils/addr2line.c
+++ b/binutils/addr2line.c
@@ -45,6 +45,9 @@ static

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler

[Nick Clifton]

Hi Jakub,

>>   Also - Tom and Pedro have raised the issue that the patch introduces
>>   a new static variable to the library that is not thread safe.  I am
>>   not sure of the best way to address this problem.  Possibly the
>>   variable could be made thread local ?  Are there any other static
> 
> Please don't.

OK. :-)

> Most of the demangler functions pass around a pointer to a struct with
> context, can't this be added in there?

Not without modifying the current demangling interface.  The problem is 
that the context structure is created for each invocation of a demangling 
function (from outside the library), and no state is preserved across 
demangling calls.  Thus in order to have a recursion limit which is 
configurable by the caller, you either need to have a global variable or 
else extend the demangling interface to include a recursion limit parameter.

I did consider just having a fixed limit, that the user cannot change, but
I thought that this might be rejected by reviewers.  (On the grounds that
different limits are appropriate to different execution environments).
Note - enabling or disabling the recursion limit is controlled by a separate
feature of the proposed patch, ie the new DMGL_RECURSE_LIMIT flag in the 
options field of the cplus_demangleXXX() functions.  But there is not enough
room in the options field to also include a recursion limit value.

Cheers
  Nick

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler

[Nick Clifton]

Hi Scott,

> Thank you for looking into this Nick. I've been staring at a few of these 
> CVEs off-and-on for a few days, and the following CVEs all look like 
> duplicates:
> 
> CVE-2018-17985: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87335
> CVE-2018-18484: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87636
> CVE-2018-18701: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87675
> CVE-2018-18700: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87681

Yes, essentially they are.  They actually trigger stack exhaustion at
different places inside libiberty, but the root cause is the same.
I am also happy to say that my proposed patch fixes *all* of these PRs.

> Perhaps some of these should be rejected?

That would nice, but I think that if the patch is accepted then the
issue should be resolved and we should stop seeing this kind of CVE.

(I must admit that my motivation for creating this patch in the first
place is that I am fed up with the amount of hassle that is involved
each time a new CVE is created.  Especially when they are essentially
all the same bug).

Cheers
  Nick

Re: RFA/RFC: Add stack recursion limit to libiberty's demangler

[Nick Clifton]

Hi Ian,

  *sigh* it is always the way.  You post a patch and five minutes later
  you find a bug in it.  In this case it turned out that I had forgotten
  that gcc has its own copy of the libiberty sources, so the bootstrap
  test that I had run did not use the patched sources.  Doh.  When I did
  rerun the bootstrap with the patched sources it failed because I had
  forgotten to use the CP_STATIC_IF_GLIBCPP_V3 macro when declaring the
  new cplus_demangle_set_recursion_limit() function.

  I am attaching a revised patch with this bug fixed, and an updated
  changelog entry as I have found a few more CVE PRs that it fixes.

  Also - Tom and Pedro have raised the issue that the patch introduces
  a new static variable to the library that is not thread safe.  I am
  not sure of the best way to address this problem.  Possibly the
  variable could be made thread local ?  Are there any other static
  variables in libiberty that face the same issue ?
  
Cheers
  Nick

libiberty/ChangeLog
2018-11-29  Nick Clifton  

PR 87681
PR 87675
PR 87636
PR 87335
* cp-demangle.c (demangle_recursion_limit): New static
variable.
(d_function_type): Add recursion counter.  If the recursion
limit is enabled and reached, return with a failure result.
(d_demangle_callback): If the recursion limit is enabled, check
for a mangled string that is so long that there is not enough
stack space for the local arrays.
(cplus_demangle_set_recursion): New function.  Sets and/or
returns the current stack recursion limit.
* cplus-dem.c (demangle_nested_args): Add recursion counter.  If
the recursion limit is enabled and reached, return with a
failure result.

Index: libiberty/cp-demangle.c
===
--- libiberty/cp-demangle.c	(revision 266657)
+++ libiberty/cp-demangle.c	(working copy)
@@ -62,6 +62,7 @@
   cplus_demangle_fill_dtor
   cplus_demangle_print
   cplus_demangle_print_callback
+  cplus_demangle_set_recursion_limit
and other functions defined in the file cp-demint.c.
 
This file also defines some other functions and variables which are
@@ -181,6 +182,9 @@
 #define cplus_demangle_init_info d_init_info
 static void d_init_info (const char *, int, size_t, struct d_info *);
 
+#define cplus_demangle_set_recursion_limit d_set_recursion_level
+static  unsigned int d_set_recursion_limit (unsigned int);
+
 #else /* ! defined(IN_GLIBCPP_V3) */
 #define CP_STATIC_IF_GLIBCPP_V3
 #endif /* ! defined(IN_GLIBCPP_V3) */
@@ -2852,21 +2856,34 @@
 static struct demangle_component *
 d_function_type (struct d_info *di)
 {
-  struct demangle_component *ret;
+  static unsigned long recursion_level = 0;
+  struct demangle_component *ret = NULL;
 
-  if (! d_check_char (di, 'F'))
-return NULL;
-  if (d_peek_char (di) == 'Y')
+  if ((di->options & DMGL_RECURSE_LIMIT)
+  && recursion_level > demangle_recursion_limit)
 {
-  /* Function has C linkage.  We don't print this information.
-	 FIXME: We should print it in verbose mode.  */
-  d_advance (di, 1);
+  /* FIXME: There ought to be a way to report that the recursion limit
+	 has been reached.  */
+  return NULL;
 }
-  ret = d_bare_function_type (di, 1);
-  ret = d_ref_qualifier (di, ret);
 
-  if (! d_check_char (di, 'E'))
-return NULL;
+  recursion_level ++;
+  if (d_check_char (di, 'F'))
+{
+  if (d_peek_char (di) == 'Y')
+	{
+	  /* Function has C linkage.  We don't print this information.
+	 FIXME: We should print it in verbose mode.  */
+	  d_advance (di, 1);
+	}
+  ret = d_bare_function_type (di, 1);
+  ret = d_ref_qualifier (di, ret);
+  
+  if (! d_check_char (di, 'E'))
+	ret = NULL;
+}
+
+  recursion_level --;
   return ret;
 }
 
@@ -6242,6 +6259,20 @@
 
   cplus_demangle_init_info (mangled, options, strlen (mangled), );
 
+  /* PR 87675 - Check for a mangled string that is so long
+ that we do not have enough stack space to demangle it.  */
+  if ((options & DMGL_RECURSE_LIMIT)
+  /* This check is a bit arbitrary, since what we really want to do is to
+	 compare the sizes of the di.comps and di.subs arrays against the
+	 amount of stack space remaining.  But there is no portable way to do
+	 this, so instead we use the recursion limit as a guide to the maximum
+	 size of the arrays.  */
+  && (unsigned long) di.num_comps > demangle_recursion_limit)
+{
+  /* FIXME: We need a way to indicate that a stack limit has been reached.  */
+  return 0;
+}
+
   {
 #ifdef CP_DYNAMIC_ARRAYS
 __extension__ struct demangle_component comps[di.num_comps];
@@ -6324,6 +6355,23 @@
   return dgs.buf;
 }
 
+/* Set a limit on the amount of recursion allowed in mangled strings.
+   Only effective if the DMGL_RECURSE_LIMIT option has been set.
+   Returns the previous value of the recu

RFA/RFC: Add stack recursion limit to libiberty's demangler

[Nick Clifton]

Hi Ian,

  Libiberty's demangler seems to be a favourite target of people looking
  to file new CVEs by fuzzing strings and I have finally gotten tired of
  reviewing them all.  So I would like to propose a patch to add support
  for placing a recursion limit on the demangling functions.

  The patch adds a new demangling option - DMGL_RECURSE_LIMIT - which
  needs to be present in order for the new code to take affect.   So
  current users of the libiberty library will not be affected[*].
  The patch also adds a new function - cplus_demangle_set_recursion_limit()
  - which can be used to probe and/or set the limit.

  When the option is in effect a few of the demangler functions will use
  static counters to make sure that they have not been called
  recursively too many times.  I only chose those functions for which I
  could find filed PRs that triggered this kind of problem.  But with
  the stack limiting framework in place it would be easy to add checks
  to other functions, should they prove necessary.

  I also encountered one binary that could trigger stack exhaustion in
  d_demangle_callback where it tries to allocate stack space to hold
  component and substitution arrays.  So the patch includes a check
  for this situation as well.
  
  There does not appear to be any error reporting framework for the
  demangler functions, so when a recursion limit is reached the
  functions just return a failure/NULL result.

  I have tested the patch with a variety of different binutils builds
  and also bootstrapped an x86_64-pc-linux-gnu toolchain.  No new
  failures were found.

  What do you think, is this approach reasonable ?

Cheers
  Nick

[*] Actually I also have a patch for the binutils to modify the
addr2line, c++filt, nm and objdump programs to make use of this new
feature, should it be accepted into libiberty.

Patches:

include/ChangeLog
2018-11-29  Nick Clifton  

* demangle.h (DMGL_RECURSE_LIMIT): Define.
(cplus_demangle_set_recursion_limit): Prototype.

libiberty/ChangeLog
2018-11-29  Nick Clifton  

PR 87675
PR 87636
* cp-demangle.c (demangle_recursion_limit): New static
variable.
(d_function_type): Add recursion counter.  If the recursion
limit is enabled and reached, return with a failure result.
(d_demangle_callback): If the recursion limit is enabled, check
for a mangled string that is so long that there is not enough
stack space for the local arrays.
(cplus_demangle_set_recursion): New function.  Sets and/or
returns the current stack recursion limit.
* cplus-dem.c (demangle_nested_args): Add recursion counter.  If
the recursion limit is enabled and reached, return with a
failure result.

diff --git a/include/demangle.h b/include/demangle.h
index b8d57cf295..426b5880aa 100644
--- a/include/demangle.h
+++ b/include/demangle.h
@@ -65,6 +65,8 @@ extern "C" {
 #define DMGL_DLANG	 (1 << 16)
 #define DMGL_RUST	 (1 << 17)	/* Rust wraps GNU_V3 style mangling.  */
 
+#define DMGL_RECURSE_LIMIT (1 << 18)	/* Enable limits on the depth of recursion in mangled strings.  */
+
 /* If none of these are set, use 'current_demangling_style' as the default. */
 #define DMGL_STYLE_MASK (DMGL_AUTO|DMGL_GNU|DMGL_LUCID|DMGL_ARM|DMGL_HP|DMGL_EDG|DMGL_GNU_V3|DMGL_JAVA|DMGL_GNAT|DMGL_DLANG|DMGL_RUST)
 
@@ -716,6 +718,15 @@ cplus_demangle_print_callback (int options,
struct demangle_component *tree,
demangle_callbackref callback, void *opaque);
 
+/* Set a limit on the amount of recursion allowed in mangled strings.
+   Only effective if the DMGL_RECURSE_LIMIT option has been set.
+   Returns the previous value of the recursion limit.
+   Ignores settings a limit of 0 or 1.
+   Note - setting the limit is not a thread-safe operation.  */
+
+extern unsigned long
+cplus_demangle_set_recursion_limit (unsigned long limit);
+  
 #ifdef __cplusplus
 }
 #endif /* __cplusplus */
diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c
index 3f2a097e7f..a6aeac58a3 100644
--- a/libiberty/cp-demangle.c
+++ b/libiberty/cp-demangle.c
@@ -568,6 +568,11 @@ static int d_demangle_callback (const char *, int,
 demangle_callbackref, void *);
 static char *d_demangle (const char *, int, size_t *);
 
+/* A limit on the amount of recursion allowed in mangled strings.
+   Only effective if the DMGL_RECURSE_LIMIT option has been set.
+   The initial value of 1024 is an arbitrary choice.  */
+static unsigned long demangle_recursion_limit = 1024;
+
 #define FNQUAL_COMPONENT_CASE\
 case DEMANGLE_COMPONENT_RESTRICT_THIS:		\
 case DEMANGLE_COMPONENT_VOLATILE_THIS:		\
@@ -2843,21 +2848,34 @@ d_ref_qualifier (struct d_info *di, struct demangle_component *sub)
 static struct demangle_component *
 d_function_type (struct d_info *di)
 {
-  struct demangle_component *ret;
+  static u

Re: RFA: Sanitize deprecation messages (PR 84195)

[Nick Clifton]

Hi Martin,

> I'm getting a bootstrap failure:

*sigh* yes - my bad.  Fortunately a patch has already been posted:

  https://gcc.gnu.org/ml/gcc-patches/2018-06/msg01039.html

And I think that it has now been approved.

Cheers
  Nick

Re: [tree.c] Replace cast to (char *) by const_cast

[Nick Clifton]

Hi Prathamesh,

> I am getting the following build error with trunk:
> ../../gcc/gcc/tree.c: In member function ‘void
> escaped_string::escape(const char*)’:
> ../../gcc/gcc/tree.c:12457:20: error: cast from type ‘const char*’ to
> type ‘char*’ casts away qualifiers [-Werror=cast-qual]
>m_str = (char *) unescaped;
> ^
> I think this is caused by r261697 in tree.c:
>   m_str = (char *) unescaped;
> 
> The patch changes it to const_cast (unescaped) which fixes the
> build for me.

I cannot approve this patch, but I can say thanks very much for catching
this problem and proposing a fix.  I guess that I must be using an old
version of g++ for my testing as this error did not show up. :-(

Cheers
  Nick

RFA: Restore ability to build zlib in a srcdir == builddir

[Nick Clifton]

Hi Guys,

  The patch below allows the zlib library to be built when the build
  directory is the same as the source directory.  This patch has been in
  the binutils/gdb sources for a while now, but unfortunately was never
  copied to gcc.  So I am trying to right that mistake now.

  OK to apply ?

Cheers
  Nick

./ChangeLog
* zlib/configure.ac: Restore old behaviour of only enabling
multilibs when a target subdirectory is defined.  This allows
building with srcdir == builddir.
* zlib/configure: Regenerate.

diff --git a/zlib/configure.ac b/zlib/configure.ac
index fb8d943905..57d6fa56b6 100644
--- a/zlib/configure.ac
+++ b/zlib/configure.ac
@@ -4,7 +4,9 @@ AC_PREREQ(2.64)
 AC_INIT
 AC_CONFIG_SRCDIR([zlib.h])
 
-AM_ENABLE_MULTILIB(, ..)
+if test -n "${with_target_subdir}"; then
+  AM_ENABLE_MULTILIB(, ..)
+fi
 
 AC_CANONICAL_SYSTEM

Re: RFA: Sanitize deprecation messages (PR 84195)

[Nick Clifton]

Hi Jeff,

  Thanks for the review.

> The docs still say "Control characters in the string will be replaced
> with spaces", but they're being escaped now.  That needs to be fixed.

Done.

> I note you overload the cast operator in your new class.  Why not just
> use an accessor?  Was this style something someone else suggested?

Yup.  My C++ foo is very weak, so I asked Martin for help, and that
was he suggested.  Is this method wrong ?



> Onward to the nits :-)

:-)

> +  char *m_str;
> +  bool  m_owned;
> 
> I don't think we try to line up variable definitions like that.

OK. Fixed.

> + escaped = (char *) xmalloc (len * 2 + 1);
> I believe that generally we want a slower growth pattern.  You'll find
> that we regularly do something like len * 3 / 2 + 1.  Seems wise here too.

Also done.

> +void
> +escaped_string::escape (const char * unescaped)
> No need for the space between the * and unescaped.

Et tu Jeff ?  Then fall Nick. :-)  [Fixed]


> +   case '\v': escaped[new_i++] = 'v'; break;
> +   default:   escaped[new_i++] = '?'; break;
> +   }
> I believe our coding standards would have the statements on new lines
> rather than on the line with the case label.

Sorted.

> So I think the biggest question here is the cast overload vs just using
> an accessor.

OK, so demonstrating my ignorance: what is the accessor solution to this 
problem ?

Cheers
  Nick

PS.  Revised patch attached in case the current solution of casting the 
operators is OK.


pr84195.patch.6
Description: Unix manual page

Re: [PATCH] [configure] Added "nfp" to the build for binutils.

[Nick Clifton]

Hi Francois,

> 2018-05-01  Francois H. Theron  
> 
>   * configure.ac: Added "nfp" target.
>   * configure: Regenerate.

I have applied this change patch to both the gcc and binutils mainline sources.

Cheers
  Nick

Re: [PATCH] RX TARGET_RTX_COSTS function

[Nick Clifton]

Hi Oleg,

> Ping.

Sorry - I am not very good at spotting RX bugs on the gcc-patches list. :-(

>> gcc/ChangeLog:
>>  * config/rx/rx.c (rx_rtx_costs): New function.
>>  (TARGET_RTX_COSTS): Override to use rx_rtx_costs.

Approved - please apply.

Cheers
  Nick

Re: plugin-api.h patch to add a new interface for linker plugins

[Nick Clifton]

Hi Cary, Hi Sriraman,

>> Ping.  Is this alright to apply now or should I wait for Stage 1?
>>
>> * plugin-api.h (ld_plugin_get_wrap_symbols): New
>>   plugin interface.
> 
> I'd say go ahead and apply the patch in binutils, and wait for Stage 1
> to sync back to GCC, unless someone there OKs it sooner.
> 
> Nick, is that OK?

Yes, that is fine with me.

Cheers
  Nick

Re: RFA: Sanitize deprecation messages (PR 84195)

[Nick Clifton]

Hi Martin,


> Since the class manages a resource it should ideally make sure it
> doesn't try to release the same resource multiple times.  I.e., its
> copy constructor and assignment operators should either "do the right
> thing" (whatever you think that is) or be made inaccessible (or declared 
> deleted in C++ 11).
> 
> For example:
> 
>   {
>     escaped_string a;
>     a.escape ("foo\nbar");
> 
>     escaped_string b (a);
>     // b destroys its m_str
>     // double free in a's destructor here
>   }

I am not sure that this can happen.  First of the escaped_string
class does not have constructor that accepts a char* argument.
(Maybe in C++ this is done automatically ?  My C++-fu is very weak).

Secondly the m_owned private field is only set to true when
the m_str field is set to a string allocated by the particular
instance of the class, and memory is only freed by the destructor 
if m_owned is true.  

So even this should work:

  {
escaped_string a,b;

a.escape ("foo\nbar");
b.escape ((const char *) a);
  }

The destructor for B will not free any memory, even though its
m_str field has been set to the same string as A, because its
m_owned field will be set to FALSE.

Cheers
  Nick

Re: RFA: Sanitize deprecation messages (PR 84195)

[Nick Clifton]

Hi David,

  Attached is a revised version of the patch which I hope addresses all 
  of your (very helpful) comments on the v3 patch.

  OK to apply once the sources are back on stage 1 ?

Cheers
  Nick

gcc/ChangeLog
2018-02-09  Nick Clifton  <ni...@redhat.com>

PR 84195
* tree.c (escaped_string): New class.  Converts an unescaped
string into its escaped equivalent.
(warn_deprecated_use): Use the new class to convert the
deprecation message, if present.
(test_escaped_strings): New self test.
(test_c_tests): Add test_escaped_strings.
* doc/extend.texi (deprecated): Add a note that the
deprecation message is affected by the -fmessage-length
option, and that control characters will be escaped.
(#pragma GCC error): Document this pragma.
(#pragma GCC warning): Likewise.
* doc/invoke.texi (-fmessage-length): Document this option's
effect on the #warning and #error preprocessor directives and
the deprecated attribute.

gcc/testsuite/ChangeLog
2018-02-09  Nick Clifton  <ni...@redhat.com>

PR 84195
* gcc.c-torture/compile/pr84195.c: New test.


pr84195.patch.4
Description: Unix manual page

Re: [RX] Fix PR 83831 -- Unused bclr, bnot, bset insns

[Nick Clifton]

Hi Oleg,

> OK for trunk?

> gcc/ChangeLog:
> 
>   PR target/83831
>   * config/rx/rx-protos.h (rx_reg_dead_or_unused_after_insn,
>   rx_copy_reg_dead_or_unused_notes, rx_fuse_in_memory_bitop): New
>   declarations.
>   (set_of_reg): New struct.
>   (rx_find_set_of_reg, rx_find_use_of_reg): New functions.
>   * config/rx/rx.c (rx_reg_dead_or_unused_after_insn,
>   rx_copy_reg_dead_or_unused_notes, rx_fuse_in_memory_bitop): New
>   functions.
>   * config/rx/rx.md (andsi3, iorsi3, xorsi3): Convert to insn_and_split.
>   Split into bitclr, bitset, bitinvert patterns if appropriate.
>   (*bitset, *bitinvert, *bitclr): Convert to named insn_and_split and
>   use rx_fuse_in_memory_bitop.
>   (*bitset_in_memory, *bitinvert_in_memory, *bitclr_in_memory): Convert
>   to named insn, correct maximum insn length.
> 
> gcc/testsuite/ChangeLog:
> 
>   PR target/83831
>   * gcc.target/rx/pr83831.c: New tests.

Approved - please apply - and thanks very much for doing this!

Cheers
  Nick

Re: RFA: Sanitize deprecation messages (PR 84195)

[Nick Clifton]

Hi David, Hi Martin,

  OK, take 3 of the patch is attached.  In this version:

  * The escaping is handled by a new class.
  * Self-tests have been added (and they helped find a bug - yay!)
  * The documentation has been extended to mention -fmessage-length's
effect on the #-directives, and to add documentation for #pragma
GCC error and #pragma GCC warning.  (Which appeared to be missing).

  I did try to add backslash characters to the regexp in the new testcase
  but I just could not find a way to persuade dejagnu to accept them.

  OK to apply ?

  (Apologies for the poor C++ coding - it is not my strong point.  I am
  an assembler level programmer at heart).

Cheers
  Nick

gcc/ChangeLog
2018-02-09  Nick Clifton  <ni...@redhat.com>

PR 84195
* tree.c (escaped_string): New class.  Converts an unescaped
string into its escaped equivalent.
(warn_deprecated_use): Use the new class to convert the
deprecation message, if present.
(test_escaped_strings): New self test.
(test_c_tests): Add test_escaped_strings.

gcc/testsuite/ChangeLog
2018-02-07  Nick Clifton  <ni...@redhat.com>

* gcc.c-torture/compile/pr84195.c: New test.



pr84195.patch.3
Description: Unix manual page

Re: RFA: Fix PR 68028: LTO error when compiling PowerPC binaries with single precision floating point

[Nick Clifton]

Hi Segher,


> I thought you were going to do a patch like the following, to make the
> e500 cores less special (they are not):

Sorry - my bad.  I defer to your patch then.  Whatever it takes to get
the BZ fixed and off the books... :-)

Cheers
  Nick

RFA: Fix PR 68028: LTO error when compiling PowerPC binaries with single precision floating point

[Nick Clifton]

Hi Segher,

  OK, here is an official submission of my patch to fix PR 68028.

  I should note that Richard Guenther feels that there is a better way
  to solve the problem - by only initializing the values once - but I
  still like my solution, so I am offering it here.

  OK to apply ?

Cheers
  Nick

gcc/ChangeLog
2018-02-08  Nick Clifton  <ni...@redhat.com>

* config/rs6000/rs6000.c (rs6000_option_override_internal):
In LTO mode prefer function attributes over command line -mcpu
setting.

Index: gcc/config/rs6000/rs6000.c
===
--- gcc/config/rs6000/rs6000.c  (revision 257282)
+++ gcc/config/rs6000/rs6000.c  (working copy)
@@ -4834,12 +4834,25 @@
 
   if (main_target_opt)
 {
-  if (main_target_opt->x_rs6000_single_float != rs6000_single_float)
-   error ("target attribute or pragma changes single precision floating "
-  "point");
-  if (main_target_opt->x_rs6000_double_float != rs6000_double_float)
-   error ("target attribute or pragma changes double precision floating "
-  "point");
+  /* PR 68028: In LTO mode the -mcpu value is passed in as a function
+attribute rather than on the command line.  So instead of checking
+for discrepancioes, we enforce the choice determined by the
+attributes.  */
+if (in_lto_p)
+  {
+rs6000_single_float = main_target_opt->x_rs6000_single_float;
+rs6000_double_float = main_target_opt->x_rs6000_double_float;
+  }
+/* There could be an 'else' statement here but it is hardly worth
+   it as the compiler will make the optimization anyway, and this
+   way we avoid indenting the code unnecessarily.  */
+
+if (main_target_opt->x_rs6000_single_float != rs6000_single_float)
+  error ("target attribute or pragma changes single precision floating 
"
+ "point");
+if (main_target_opt->x_rs6000_double_float != rs6000_double_float)
+  error ("target attribute or pragma changes double precision floating 
"
+ "point");
 }
 
   rs6000_always_hint = (rs6000_tune != PROCESSOR_POWER4

Re: RFA: Sanitize deprecation messages (PR 84195)

[Nick Clifton]

Hi David,

>> +  /* PR 84195: Replace control characters in the message with their
>> + escaped equivalents.  Allow newlines if -fmessage-length has
>> + been set to a non-zero value.  
> 
> I'm not quite sure why we allow newlines in this case, sorry.

Because the documentation for -fmessage-length says:

  Try to format error messages so that they fit on lines 
  of about N characters.  If N is zero, then no 
  line-wrapping is done; each error message appears on a 
  single line.  This is the default for all front ends.

So with a non-zero message length, multi-line messages are allowed.

At least that was my understanding of the option.

Thanks for the patch review.  I will get onto fixing the points you
raised today.

Cheers
  Nick

Re: RFA: Sanitize deprecation messages (PR 84195)

[Nick Clifton]

Hi Martin, Hi David,

OK - attached is a new patch that:

  * Replaces control characters with their escape equivalents.
  * Includes a testcase.

I was not sure what to do about the inconsistencies between the
behaviour of #warning and #pragma GCC warning, (and the error
equivalents), so I decided to leave it for now.  Fixing them
will require mucking about in the C preprocessor library, which
I did not fancy doing at the moment.

So, is this enhanced patch OK now ?

Cheers
  Nick

gcc/ChangeLog
2018-02-07  Nick Clifton  <ni...@redhat.com>

PR 84195
* tree.c (warn_deprecated_use): Replace control characters in
deprecation messages with escape sequences.

gcc/testsuite/ChangeLog
2018-02-07  Nick Clifton  <ni...@redhat.com>

* gcc.c-torture/compile/pr84195.c: New test.


pr84195.patch.2
Description: Unix manual page

Re: PING [PATCH] -mjsr option bug fix

[Nick Clifton]

Hi Sebastian,

> +2018-01-05  Sebastian Perta  
> +
> + * config/rx/constraints.md: added new constraint
> CALL_OP_SYMBOL_REF
> + to allow or block "symbol_ref" depending on value of TARGET_JSR
> + * config/rx/rx.md: use CALL_OP_SYMBOL_REF in call_internal and
> + call_value_internal insns
> +

Approved - please apply.

Note for the future - it is usually best to provide changelog entries
as plain text, rather than a context diff, as they almost never apply
cleanly...

Cheers
  Nick

Re: PING [PATCH] RX movsicc degrade fix

[Nick Clifton]

Hi Sebastian,

  Sorry for missing this one.  If it helps in the future, feel free to ping me 
directly.

> +2018-01-09  Sebastian Perta  
> +
> + *config/rx.md: updated "movsicc" expand to be matched by GCC
> + *testsuite/gcc.target/rx/movsicc.c: new test case

Approved - please apply.

Cheers
  Nick

Re: RFA: Sanitize deprecation messages (PR 84195)

[Nick Clifton]

Hi Martin,

> My only suggestions are to consider how control characters and
> excessively messages are handled in other contexts and adopt
> the same approach here.

Are there other places where user-supplied messages are displayed by gcc ?


> In the tests I did, control characters
> were mostly escaped (e.g., as \n or as \x0a) rather than replaced
> with spaces. 

Ooo - what tests were these ?

I agree that it there is a standard way to handle these characters 
then the deprecated attribute ought to do the same.  (Plus hopefully
there is already a function in gcc to do this kind of processing,
so the deprecation code can just call it).


> I haven't thought too hard about how excessively
> long messages should be handled, or about the interactions with
> -fmessage-length= (i.e., if messages longer than the limit should
> be broken up.)

I believe that this will happen automatically.  The diagnostic display
routine will automatically insert newlines into the output if the 
message length is non-zero, and the message extends past this limit.
(Well provided that the message contains spaces.  The newlines are
only inserted in place of space characters so a very long, single
word message will not be split...)

Cheers
  Nick

Re: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Nick Clifton]

Hi Igor,

>>   Attached is a potential patch for PR 84145:
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84145

> Coincidentally, I have worked on the same patch.

Great minds, etc :-)

> Please look at the patch, I uploaded it to the bug. The main differences are
> 
> - updated the output messages to be more informative;
> - updated  the tests and add couple of new tests to check the messages;
> - fixed a typo in the doc file related to fcf-protection;
> 
> I am ok with the changes in i386.c but would like to update the messages. 
> Could you incorporate my changes and proceed? Or would you like me to finish 
> the fix?

If you are happy to finish the fix then please do so.  Your fix is
more thorough than mine, so I am happy to see it go on.  Although
I should say that I am not an x86 maintainer, so I cannot approve
it.

Cheers
  Nick

RFA: Sanitize deprecation messages (PR 84195)

[Nick Clifton]

Hi Martin, Hi Dodji, Hi David,

  PR 84195 makes the point that deprecation messages do not follow the
  formatting guidelines set out by the -fmessage-length option, and that
  they could even contain unwanted control characters:
  
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84195

  Below is my attempt at a patch to fix this.  It is not very clever,
  just replacing the control characters with spaces, and doing it each
  time that a deprecation warning is displayed.  But I figured that
  such warnings are going to be rare and do not need to be efficiently
  handled.  So I choose to keep it simple. :-)

  No regressions with an x86_64-pc-linux-gnu toolchain.

  OK to apply ?

Cheers
  Nick

gcc/ChangeLog
2018-02-05  Nick Clifton  <ni...@redhat.com>

PR 84195
* tree.c (warn_deprecated_use): Sanitize deprecation messages.

Index: gcc/tree.c
===
--- gcc/tree.c  (revision 257389)
+++ gcc/tree.c  (working copy)
@@ -12436,6 +12436,39 @@
   else
 msg = NULL;
 
+  char * new_msg = NULL;
+  if (msg)
+{
+  unsigned int i; /* FIXME: Do we need to check to see if msg is longer
+than 2^32 ?  */
+
+  /* PR 84195: Sanitize the message:
+
+If -fmessage-length is set to 0 then replace newline characters with
+spaces.  Replace other non-printing ASCII characters with spaces too.
+
+We do this check here, rather than where the deprecated attribute is
+recorded because the setting of -fmessage-length can be changed
+between those two locations.  */
+  for (i = 0; i < strlen (msg); i++)
+   {
+ char c = msg[i];
+ 
+ if (! ISCNTRL (c))
+   continue;
+
+ if (c != '\n' || ! pp_is_wrapping_line (global_dc->printer))
+   {
+ if (new_msg == NULL)
+   new_msg = xstrdup (msg);
+ new_msg [i] = ' ';
+   }
+   }
+
+  if (new_msg)
+   msg = new_msg;
+}
+
   bool w;
   if (DECL_P (node))
 {
@@ -12505,6 +12538,8 @@
}
}
 }
+
+  free (new_msg);
 }
 
 /* Return true if REF has a COMPONENT_REF with a bit-field field declaration

RFA: PR 84154: Fix checking -mibt and -mshstk options for control flow protection

[Nick Clifton]

Hi H.J.

  Attached is a potential patch for PR 84145:
  
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84145

  The problem was that the code to check that the --mibt and/or -mshstk
  options have been correctly enabled for control flow protection did
  not take into account that the wrong option might have been enabled.

  So the patch inverts the test, looking at the value of
  flag_cf_protection first and then checking to see if the needed x86
  specific options have been enabled.  This gives results like this:


   % gcc -c main.c
   % gcc -c main.c -fcf-protection=full
cc1: error: '-fcf-protection=full' requires CET support on this target. Use 
-mcet or both of -mibt and -mshstk options to enable CET
   % gcc -c main.c -fcf-protection=full -mcet
   % gcc -c main.c -fcf-protection=full -mibt
cc1: error: '-fcf-protection=full' requires CET support on this target. Use 
-mcet or both of -mibt and -mshstk options to enable CET
   % gcc -c main.c -fcf-protection=full -mibt -mshstk
   % gcc -c main.c -fcf-protection=branch
cc1: error: '-fcf-protection=branch' requires CET support on this target. Use 
-mcet or -mibt to enable CET
   % gcc -c main.c -fcf-protection=branch -mcet
   % gcc -c main.c -fcf-protection=branch -mibt
   % gcc -c main.c -fcf-protection=branch -mshstk
cc1: error: '-fcf-protection=branch' requires CET support on this target. Use 
-mcet or -mibt to enable CET
   % gcc -c main.c -fcf-protection=return
cc1: error: '-fcf-protection=return' requires CET support on this target. Use 
-mcet or -mshstk to enable CET
   % gcc -c main.c -fcf-protection=return -mcet
   % gcc -c main.c -fcf-protection=return -mibt
cc1: error: '-fcf-protection=return' requires CET support on this target. Use 
-mcet or -mshstk to enable CET
   % gcc -c main.c -fcf-protection=return -mshstk
   %
   
  What do you think ?  Is the patch OK for the mainline ?

Cheers
  Nick

gcc/ChangeLog
2018-02-05  Nick Clifton  <ni...@redhat.com>

PR 84145
* config/i386/i386.c (ix86_option_override_internal): Rework
checks for -fcf-protection and -mibt/-mshstk.

Index: gcc/config/i386/i386.c
===
--- gcc/config/i386/i386.c  (revision 257389)
+++ gcc/config/i386/i386.c  (working copy)
@@ -4915,30 +4915,43 @@
   /* Do not support control flow instrumentation if CET is not enabled.  */
   if (opts->x_flag_cf_protection != CF_NONE)
 {
-  if (!(TARGET_IBT_P (opts->x_ix86_isa_flags2)
-   || TARGET_SHSTK_P (opts->x_ix86_isa_flags)))
+  switch (flag_cf_protection)
{
- if (flag_cf_protection == CF_FULL)
+   case CF_NONE:
+ break;
+   case CF_BRANCH:
+ if (! TARGET_IBT_P (opts->x_ix86_isa_flags2))
{
- error ("%<-fcf-protection=full%> requires CET support "
-"on this target. Use -mcet or one of -mibt, "
-"-mshstk options to enable CET");
+ error ("%<-fcf-protection=branch%> requires CET support "
+"on this target. Use -mcet or -mibt to enable CET");
+ flag_cf_protection = CF_NONE;
+ return false;
}
- else if (flag_cf_protection == CF_BRANCH)
+ break;
+   case CF_RETURN:
+ if (! TARGET_SHSTK_P (opts->x_ix86_isa_flags))
{
- error ("%<-fcf-protection=branch%> requires CET support "
-"on this target. Use -mcet or one of -mibt, "
-"-mshstk options to enable CET");
+ error ("%<-fcf-protection=return%> requires CET support "
+"on this target. Use -mcet or -mshstk to enable CET");
+ flag_cf_protection = CF_NONE;
+ return false;
}
- else if (flag_cf_protection == CF_RETURN)
+ break;
+   case CF_FULL:
+ if (   ! TARGET_IBT_P (opts->x_ix86_isa_flags2)
+|| ! TARGET_SHSTK_P (opts->x_ix86_isa_flags))
{
- error ("%<-fcf-protection=return%> requires CET support "
-"on this target. Use -mcet or one of -mibt, "
+ error ("%<-fcf-protection=full%> requires CET support "
+"on this target. Use -mcet or both of -mibt and "
 "-mshstk options to enable CET");
+ flag_cf_protection = CF_NONE;
+ return false;
}
- flag_cf_protection = CF_NONE;
- return false;
+ break;
+   default:
+ gcc_unreachable ();
}
+
   opts->x_flag_cf_protection =
(cf_protection_level) (opts->x_flag_cf_protection | CF_SET);
 }

Re: [RX] Fix PR 81819

[Nick Clifton]

Hi Oleg,

> gcc/ChangeLog:
>   PR target/81819
>   * config/rx/rx.c (rx_is_restricted_memory_address):
>   Handle SUBREG case.

Go ahead make my day^H^H^H^H^H^H

Approved - please apply.

Cheers
  Nick

Re: [RX] Fix PR 81821

[Nick Clifton]

Hi Oleg,

> OK for trunk and GCC 7?

Approved.  Do you have access to the repository, or would you like me to apply 
the patch for you ?

Cheers
  Nick

Re: [PATCH 0/3] Add __builtin_load_no_speculate

[Nick Clifton]

Hi Guys,

  It seems to me that it might be worth taking a step back here,
  and consider adding a security framework to gcc.  Mitigations
  for CVEs in the past have resulted in individual patches being
  added to gcc, oftern in a target specific manner, and with no
  real framework to support them, document them, or indicate to
  an external tool that they have been applied.

  In addition security fixes often result in the generation of
  less optimal code, and so it might be useful to have a way to
  tell other parts of gcc that a given particular sequence should
  not be altered.

  Not that I am an expert in this area, but I do think that it is
  something that should be discussed...

Cheers
  Nick

Re: [PATCH] [MSP430] Pass -mcode/data-region to the linker and assembler

[Nick Clifton]

Hi Jozef,

> The changes made in a series of binutils patches
> (https://sourceware.org/ml/binutils/2017-08/msg00274.html)
> to ld and gas require the -mcode/data-region options to be propagated
> from gcc.
> 
> The attached patch adds that functionality.

Approved and applied.

Cheers
  Nick

Re: [PATCH] [MSP430] Read mcu data from file instead of hardcoded data

[Nick Clifton]

Hi Jozef,

> If the file is found then it is parsed, and the device passed to the
> mmcu option is searched for. If the file or device aren't found, then
> GCC uses the old hard-coded data instead.

Whilst testing this patch with -mlarge enabled on the command line, I
encountered this (new) failure:

  cc1: error: -mlarge requires a 430X-compatible -mmcu=

  compiler exited with status 1
  FAIL: gcc.target/msp430/no_devices_warn_msp430.c (test for excess errors)

I think that you might need to extend the skip list in the test...

Cheers
  Nick

Re: [PATCH] [MSP430] [PR80993] Prevent lto removing interrupt handlers

[Nick Clifton]

Hi Jozef,

> As reported in PR80993, enabling lto causes interrupt handlers to be
> removed. This patch marks interrupt handlers as used, preventing them
> from being optimized out.
> 
> If the attached patch is acceptable, I would appreciate if someone could
> commit it for me, as I do not have write access.

Approved and applied.  Thanks for fixing this bug.

Cheers
  Nick

Re: [PATCH 2/2] [MSP430] Fix issues handling .persistent attribute (PR 78818)

[Nick Clifton]

Hi Jozef,

> Sorry, didn't mention in that last post that I don't have write access,
> could someone please apply this for me.

Applied.  Sorry about the delay (again).

Cheers
  Nick

Re: [PATCH 2/2] [MSP430] Fix issues handling .persistent attribute (PR 78818)

[Nick Clifton]

Hi Jozef,

> Ok for trunk and gcc-7-branch?

Approved - please apply (to both).

Cheers
  Nick

Re: RFA: Enhance information recorded by -frecord-gcc-switches

[Nick Clifton]

Hi Jakub,


>> Regardless, the point of this patch is to record which options were enabled, 
>> via
>> whatever route, in the binaries.  This can be useful to users, or 
>> distributors,
>> who want to check that, for example, a specific security option was enabled, 
>> or
>> that a particular a particular optimization was run.
> 
> And that again doesn't tell you whether the particular optimization pass was
> run, just that some flag variable was zero or non-zero or had some other
> value.  The decisions in the compiler are more complex and keep changing
> between compiler versions.  For one particular compiler version, -O2 vs. -O1
> if that is what was originally used to compile something is all you need,
> that implies a particular behavior, set of options and their interactions.
> For comparisons between different compiler versions, some of the options
> are ignored, others are added, others change meaning, and expanding the list
> of guarded options isn't really useful.

OK -so we need some other way of recording what optimization passes were 
actually 
run.  Fortunately I have something in mind.

Patch withdrawn.

Cheers
  Nick

Re: RFA: Enhance information recorded by -frecord-gcc-switches

[Nick Clifton]

Hi Richard,

>>The -frecord-gcc-switches option records the gcc command line.  It
>>does not however expand options like -O2 into the optimizations that
>>this enables.

> 
> I think individually enabled passes by -On are not really useful information
> as you generally cannot replace -On by a set of other switches on the
> command-line.

Is that true ?  I always thought that -O2 could be duplicated by using a (very
long) set of individual command line options.

Regardless, the point of this patch is to record which options were enabled, via
whatever route, in the binaries.  This can be useful to users, or distributors,
who want to check that, for example, a specific security option was enabled, or
that a particular a particular optimization was run.

Cheers
  Nick

RFA: Enhance information recorded by -frecord-gcc-switches

[Nick Clifton]

Hi Guys,

  The -frecord-gcc-switches option records the gcc command line.  It
  does not however expand options like -O2 into the optimizations that
  this enables.  Thus if a user wants to know if a specific optimization
  was used when creating an object file, (or library or executable),
  they will have to reverse engineer the compilation process.  Which may
  or may not be possible.

  The attached patch is a proposal to address this problem by making
  -frecord-gcc-switches also record all the enabled options.  This does
  make object files bigger, but this cannot be helped.  The enhancement
  is not enabled by default however, instead a second command line
  option must be used.  In a possibly contentious move I chose to reuse
  the -fverbose-asm option, rather than creating a new one.  I did this
  because a) it simplifies the patch, b) we have more than enough switch
  recording options already, c) it does not conflict with the current
  use of -fverbose-asm and d) it ties in nicely with the name of the
  option.

  Tested, with no regressions on an x86_64-pc-linux-gnu target, and
  built for a variety of other targets.

  OK to apply ?

Cheers
  Nick

gcc/ChangeLog
2017-06-08  Nick Clifton  <ni...@redhat.com>

* varasm.c (dump_enabled_to_asm_out_file): New function.  Prints
enabled options to the asm_out_file.
(elf_record_gcc_switches): If verbose-asm is set then also dump
all enabled options to the asm file.
* toplec.c (print_switch_values): Convert from static to global.
* doc/invoke.texi (-fverbose-asm): Mention its effect on the
-frecord-gcc-switches option.
(-frecord-gcc-switches): Refactor the description and add details
of how -fverbose-asm modifies its behaviour.

Index: gcc/doc/invoke.texi
===
--- gcc/doc/invoke.texi	(revision 249007)
+++ gcc/doc/invoke.texi	(working copy)
@@ -12281,10 +12281,13 @@
 who actually need to read the generated assembly code (perhaps while
 debugging the compiler itself).
 
-@option{-fno-verbose-asm}, the default, causes the
-extra information to be omitted and is useful when comparing two assembler
-files.
+This switch can also be used to modify the behaviour of the
+@option{-frecord-gcc-switches} switch, making it record extra
+information in the object file.
 
+@option{-fno-verbose-asm}, the default, causes the extra information
+to be omitted and is useful when comparing two assembler files.
+
 The added comments include:
 
 @itemize @bullet
@@ -12370,17 +12373,26 @@
 
 @item -frecord-gcc-switches
 @opindex frecord-gcc-switches
-This switch causes the command line used to invoke the
-compiler to be recorded into the object file that is being created.
-This switch is only implemented on some targets and the exact format
-of the recording is target and binary file format dependent, but it
-usually takes the form of a section containing ASCII text.  This
-switch is related to the @option{-fverbose-asm} switch, but that
-switch only records information in the assembler output file as
-comments, so it never reaches the object file.
-See also @option{-grecord-gcc-switches} for another
-way of storing compiler options into the object file.
+This switch causes the command line used to invoke the compiler to be
+recorded into the object file that is being created.  This switch is
+only implemented on some targets and the exact format of the recording
+is target and binary file format dependent, but it usually takes the
+form of a section containing ASCII text.
 
+The related switch @option{-fverbose-asm} switch performs a similar
+task but it only records the information in the assembler output file
+as comments, so it never reaches the object file.
+
+If both @option{-fverbose-asm} and @option{-frecord-gcc-switches} are
+enabled together then @option{-frecord-gcc-switches} will record all
+enabled switches, not just those specified on the command line.  Thus
+if the command line includes @option{-O2} then all optimizations
+enabled by that switch will be recorded in the object file, along with
+the presence of @option{-O2} itself.
+
+See also @option{-grecord-gcc-switches} for another way of storing
+compiler options into an object file.
+
 @item -fpic
 @opindex fpic
 @cindex global offset table
Index: gcc/toplev.c
===
--- gcc/toplev.c	(revision 249007)
+++ gcc/toplev.c	(working copy)
@@ -809,7 +809,9 @@
Each line begins with INDENT and ends with TERM.
Each switch is separated from the next by SEP.  */
 
-static void
+void print_switch_values (print_switch_fn_type);
+
+void
 print_switch_values (print_switch_fn_type print_fn)
 {
   int pos = 0;
Index: gcc/varasm.c
===
--- gcc/varasm.c	(revision 249007)
+++ gcc/varasm.c	(working copy)
@@ -7545,6 +7545,32 @@
   v.release ();
 }
 
+/* Prin

Re: [PATCH 2/2] [MSP430] Fix issues handling .persistent attribute (PR 78818)

[Nick Clifton]

Hi Jozef,
   
> Attached patch with the string wrapped in G_().

When I applied this patch to the sources and ran the new test, I encountered
an internal compiler error:

 msp430-elf/gcc/xgcc [...] pr78818-auto-warn.c [...]
 [...]
 gcc/testsuite/gcc.target/msp430/pr78818-auto-warn.c: In function 'main':

 gcc/testsuite/gcc.target/msp430/pr78818-auto-warn.c:10:3: internal compiler 
error: in get, at cgraph.h:403

  0xd30d3b symtab_node::get(tree_node const*)
gcc/current/gcc/cgraph.h:400

  0xd30d3b decl_section_name(tree_node const*)
gcc/current/gcc/tree.c:700

  0xd9ff22 msp430_data_attr
 gcc/current/gcc/config/msp430/msp430.c:1998


 It seems that there is a problem with calling the DECL_SECTION_NAME macro on 
the
 line just before your new code.  Are you able to reproduce this problem ?

Cheers
  Nick

  

Re: [PATCH] [MSP430] PR78838: Do not add section name prefixes when section name is .lowtext

[Nick Clifton]

Hi Josef,

  Thanks for reporting this problem, and providing a patch to fix it.

  I have checked your patch in, along with one, very very minor change
  to the formatting of the comment in gen_prefix().

Cheers
  Nick

Re: PR translation/80280

[Nick Clifton]

Hi Martin,

> Rats!  I ran into this when building for sparcv9-solaris2.11 but
> didn't look at the cause of the error carefully enough to recognize
> it was caused by my change so I just raised 80673 for it.  It didn't
> occur to me that targets defined their own format extensions like
> this.   What a tangled mess.

Oh go on - if it was easy you would be bored...

> The fix should be trivial.  My change added just a single member:
> format_flag_spec::quoting.  I can take care of it today, along
> with bug 80673.

Thanks very much!

Cheers
  Nick

Re: PR translation/80280

[Nick Clifton]

Hi Martin,

  I am currently unable to build gcc for the x86_64-pc-cygwin target
  because gcc/config/i386/msformat-c.c uses the format_flag_spec struct
  but it has not been updated with the new field. :-(  For example:

gcc/config/i386/msformat-c.c
gcc/config/i386/msformat-c.c:52:1: error: cannot convert 
'format_std_version' to 'const char*' in initialization
gcc/config/i386/msformat-c.c:52:1: warning: missing initializer for member 
'format_flag_spec::std'
gcc/current/gcc/config/i386/msformat-c.c:52:1: error: cannot convert 
'format_std_version' to 'const char*' in initial

  Do you have time to fix this ?  If not, please could you tell me which
  of the fields in the struct is new, and how it ought to be
  initialised.

Cheers
  Nick
  

Re: install.texi and arm

[Nick Clifton]

Hi Guys,

>>> References to dependencies on really, really old versions of
>>> binutils (talking 10+ years here) which I think we can remove.

>>ARM-family processors.  Subtargets that use the ELF object format
>>require GNU binutils 2.13 or newer.  Such subtargets include:

>> Okay to yank this?
> 
> Fine by me.

Me too.

> Nick, do you have any objections to this?

None.  Are you intending to replace the requirement with a more recent
version of the binutils, or just remove the requirement entirely ?

Cheers
  Nick

RFA: Patch for ARM PR77770

[Nick Clifton]

Hi Richard, Hi Ramana,

  The patch below is a simple fix for PR0.  I am not really
  expecting you to agree with it, but I thought that it was worth
  posting so that this PR could be looked at again and maybe a better
  patch found.  (Plus I am trying to close PRs so that the gcc 7 branch
  will happen...)

  The patch is simple - it disparages the SF-mode store alternative in
  the thumb1_movsf_insn just enough to break the infinite load/store
  cycle being triggered in reload.  The patch does not introduce any
  regressions, although I suspect it might affect code quality for
  thumb1.  (I have not checked this).  Nor have I gone deep into why
  reload is generating an infinite cycle of SF load/store insns.  That
  was a bit beyond me.  But the patch works and maybe that is enough.

  Obviously, if you think that this patch is OK, I would also create a
  new ARM specific gcc testsuite entry based upon the simplified test in
  the PR.

  So - OK to apply ?
  
Cheers
  Nick

Index: gcc/config/arm/thumb1.md
===
--- gcc/config/arm/thumb1.md(revision 244853)
+++ gcc/config/arm/thumb1.md(working copy)
@@ -865,7 +865,7 @@
(set_attr "conds" "clob,nocond,nocond,nocond,nocond")])
 ;;; ??? This should have alternatives for constants.
 (define_insn "*thumb1_movsf_insn"
-  [(set (match_operand:SF 0 "nonimmediate_operand" "=l,l,>,l, m,*r,*h")
+  [(set (match_operand:SF 0 "nonimmediate_operand" "=l,l,>,l, ?m,*r,*h")
(match_operand:SF 1 "general_operand"  "l, >,l,mF,l,*h,*r"))]
   "TARGET_THUMB1
&& (   register_operand (operands[0], SFmode)

RFA: Update XFAIL comments in

[Nick Clifton]

Hi Guys,

  I would like to close out PR 70681 by applying the patch below.  It
  updates the XFAIL comments in the two affected tests, explaining why
  the check for shrink-wrapping will fail.  There is nothing actually
  wrong here.  The shrink wrapping optimization is working and the
  targets are not broken, it is just that, for these particular test
  cases, for these specific architectures (ARM, PPC), the unshrink-
  wrapped code is actually smaller than the shrink wrapped version.

  So - is it OK to apply the patch ?

Cheers
  Nick

gcc/testsuite/ChangeLog
2017-01-20  Nick Clifton  <ni...@redhat.com>

* gcc.dg/pr10474.c: Update XFAIL comment to explain why the
check for shrink-wrapping can be expected to fail.
* gcc.dg/vect/vect-strided-a-u8-i2-gap.c: Likewise.

Index: gcc/testsuite/gcc.dg/ira-shrinkwrap-prep-2.c
===
--- gcc/testsuite/gcc.dg/ira-shrinkwrap-prep-2.c	(revision 244691)
+++ gcc/testsuite/gcc.dg/ira-shrinkwrap-prep-2.c	(working copy)
@@ -31,5 +31,7 @@
 
 /* { dg-final { scan-rtl-dump "Will split live ranges of parameters" "ira"  } } */
 /* { dg-final { scan-rtl-dump "Split live-range of register" "ira"  } } */
-/* XFAIL due to PR70681.  */ 
+/* The XFAILs are because these targets produce better code without
+   shrinkwrapping, and hence the optimization is not triggered.  See
+   PR70681 for more details.  */
 /* { dg-final { scan-rtl-dump "Performing shrink-wrapping" "pro_and_epilogue" { xfail arm*-*-* powerpc*-*-* } } } */
Index: gcc/testsuite/gcc.dg/pr10474.c
===
--- gcc/testsuite/gcc.dg/pr10474.c	(revision 244691)
+++ gcc/testsuite/gcc.dg/pr10474.c	(working copy)
@@ -12,5 +12,7 @@
 	}
 }
 
-/* XFAIL due to PR70681.  */ 
+/* The XFAILs are because these targets produce better code without
+   shrinkwrapping, and hence the optimization is not triggered.  See
+   PR70681 for more details.  */
 /* { dg-final { scan-rtl-dump "Performing shrink-wrapping" "pro_and_epilogue"  { xfail arm*-*-* powerpc*-*-* } } } */

Re: [PATCH] Fix PR78189

[Nick Clifton]

Hi Guys,

  [I have been asked to look at this PR in the hopes that it can be
  fixed soon and so no longer act as a blocker for the gcc 7 branch].

  It seems to me that Richard's proposed patch does work:

https://gcc.gnu.org/ml/gcc-patches/2016-11/msg00909.html

  The only problem is that the check_effective_target_vect_hw_misalign
  proc is always returning 0 (or false) for ARM, even when unaligned
  vectors are supported.  This is why Richard's patch introduces a new
  failure for the arm-* targets.

  So what I would like to suggest is an extended patch (attached) which
  also updates the check_effective_target_vect_hw_misalign proc to use
  the check_effective_target_arm_vect_no_misalign proc.  With this patch
  applied not only does the gcc.dg/vect/vect-strided-a-u8-i2-gap.c test
  for both big-endian and little-endian arm targets, but there is also a
  significant reduction in the number of failures in the gcc.dg/vect
  tests overall:

   Little Endian ARM:
< # of expected passes  3275
< # of unexpected failures  63
< # of unexpected successes 125
< # of expected failures123
< # of unsupported tests153
---
> # of expected passes  3448
> # of unexpected failures  2
> # of unexpected successes 14
> # of expected failures131
> # of unsupported tests151

  Big Endian ARM:
< # of expected passes  2995
< # of unexpected failures  269
< # of unexpected successes 21
< # of expected failures128
---
> # of expected passes  3037
> # of unexpected failures  127
> # of unexpected successes 24
> # of expected failures228

  Which looks like a win to me.  So - any objections to my applying this
  patch and then closing the PR ?

Cheers
  Nick

gcc/ChangeLog
2017-01-20  Richard Biener  <rguent...@suse.de>
Nick Clifton  <ni...@redhat.com>

PR testsuite/78421
* lib/target-supports.exp (check_effective_target_vect_hw_misalign):
If the target is ARM return the result of the
check_effective_target_arm_vect_no_misalign proc.
* gcc.dg/vect/vect-strided-a-u8-i2-gap.c: If the target does not
support unaligned vectors then only expect one of the loops to be
unrolled.

Index: gcc/testsuite/gcc.dg/vect/vect-strided-a-u8-i2-gap.c
===
--- gcc/testsuite/gcc.dg/vect/vect-strided-a-u8-i2-gap.c	(revision 244691)
+++ gcc/testsuite/gcc.dg/vect/vect-strided-a-u8-i2-gap.c	(working copy)
@@ -71,5 +71,6 @@
   return 0;
 }
 
-/* { dg-final { scan-tree-dump-times "vectorized 2 loops" 1 "vect"  { target vect_strided2 } } } */
+/* { dg-final { scan-tree-dump-times "vectorized 1 loops" 1 "vect"  { target { vect_strided2 && { ! vect_hw_misalign } } } } } */
+/* { dg-final { scan-tree-dump-times "vectorized 2 loops" 1 "vect"  { target { vect_strided2 && vect_hw_misalign } } } } */
   
Index: gcc/testsuite/lib/target-supports.exp
===
--- gcc/testsuite/lib/target-supports.exp	(revision 244691)
+++ gcc/testsuite/lib/target-supports.exp	(working copy)
@@ -5732,6 +5732,9 @@
 	 || ([istarget mips*-*-*] && [et-is-effective-target mips_msa]) } {
 	  set et_vect_hw_misalign_saved($et_index) 1
 	}
+	if { [istarget arm*-*-*] } {
+	set et_vect_hw_misalign_saved($et_index) [check_effective_target_arm_vect_no_misalign]
+	}
 }
 verbose "check_effective_target_vect_hw_misalign:\
 	 returning $et_vect_hw_misalign_saved($et_index)" 2

RFA: Fail gracefully when registering info for an unknown plugin

[Nick Clifton]

Hi Guys,

  Whilst experimenting with writing a plugin for gcc I discovered that
  I could cause a segfault if I attempted to register a PLUGIN_INFO
  callback with any name other than the name of the plugin.  The
  attached patch fixes this problem and produces an error message
  instead.

  OK to apply ?

Cheers
  Nick

gcc/ChangeLog
2016-10-20  Nick Clifton  <ni...@redhat.com>

* plugin.c (register_plugin_info): Produce an error message if the
plugin is not found in the hash table.

Index: gcc/plugin.c
===
--- gcc/plugin.c(revision 241361)
+++ gcc/plugin.c(working copy)
@@ -330,7 +330,15 @@
 register_plugin_info (const char* name, struct plugin_info *info)
 {
   void **slot = htab_find_slot (plugin_name_args_tab, name, NO_INSERT);
-  struct plugin_name_args *plugin = (struct plugin_name_args *) *slot;
+  struct plugin_name_args *plugin;
+
+  if (slot == NULL)
+{
+  error ("unable to register info for plugin '%s' - plugin name not found",
+name);
+  return;
+}
+  plugin = (struct plugin_name_args *) *slot;
   plugin->version = info->version;
   plugin->help = info->help;
 }

Re: [PATCH, nds32] Enable GDB building for nds32*-*-* target.

[Nick Clifton]

Hi Yan-Ting,

> After contributing our porting for GDB, we need a patch to enable GDB 
> building.
> 
> Is this patch OK for commit?

It is.  I have applied the patch to the binutils sources.

Please note however that the top level configure and configure.ac files are 
shared
with the gcc project, so a change like this needs to be enacted there as well.  
I
have taken the liberty of adding gcc-patches to the CC list for this email, and
just to keep things simple, I have gone ahead and applied your patch to the gcc
sources as well.

> 2016-07-18  Yan-Ting Lin  
> 
> * configure.ac (nds32*-*-*): Remove entry to enable gdb.
> * configure: Regenerated.

Approved and applied.

Cheers
  Nick

Commit: M32R: Build crtinit.o and crtfini.o

[Nick Clifton]

Hi Guys,

  I am applying the patch below to fix a long standing snafu for the
  m32r target where the files crtinit.o and crtfini.o were not being
  built along with the rest of libgcc.

  Tested with no regressions and a lot of test case fixes using an
  m32r-elf toolchain.

Cheers
  Nick

libgcc/ChangeLog
2016-07-19  Nick Clifton  <ni...@redhat.com>

* config.host (m32r): Add m32r/t-m32r to tmake_file.
Add crtinit.o and crtfini.o to extra_parts.

Index: libgcc/config.host
===
--- libgcc/config.host  (revision 238477)
+++ libgcc/config.host  (working copy)
@@ -787,7 +787,8 @@
 tmake_file="lm32/t-lm32 lm32/t-uclinux t-libgcc-pic t-softfp-sfdf 
t-softfp"
;;  
 m32r-*-elf*)
-   tmake_file=t-fdpbit
+   tmake_file="$tmake_file m32r/t-m32r t-fdpbit"
+   extra_parts="$extra_parts crtinit.o crtfini.o"
;;
 m32rle-*-elf*)
tmake_file=t-fdpbit

Re: RFA: Generate normal DWARF DW_LOC descriptors for non integer mode pointers

[Nick Clifton]

Hi Jeff,

> I can buy that ;-)   OK with a suitable ChangeLog entry.

Thanks!  Checked in with this changelog entry.

Cheers
  Nick

gcc/ChangeLog
2016-06-22  Nick Clifton  <ni...@redhat.com>

* dwarf2out.c (scompare_loc_descriptor): Use SCALAR_INT_MODE_P() in
place of GET_MODE_CLASS() == MODE_INT, so that partial integer
modes are accepted as well.
(ucompare_loc_descriptor): Likewise.
(minmax_loc_descriptor): Likewise.
(clz_loc_descriptor): Likewise.
(popcount_loc_descriptor): Likewise.
(bswap_loc_descriptor): Likewise.
(rotate_loc_descriptor): Likewise.
(mem_loc_descriptor): Likewise.
(loc_descriptor): Likewise.

Commit: MSP430: Rename entries in option enums

[Nick Clifton]

Hi Guys,

  I recently noticed that the MSP430 backend uses some pretty generic
  names for the enum values of its hardware multiply and memory region
  options.  This could possibly cause problems if these names are used
  elsewhere, so I have decided to check in the patch below to fix this.

  Tested with no regressions on an msp430-elf toolchain.

Cheers
  Nick

gcc/ChangeLog
2016-06-16  Nick Clifton  <ni...@redhat.com>

* config/msp430/msp430-opts.h (msp430_hwmult_types): Add
MSP430_HWMULT_ prefix to enum values.
(msp430_regions): Add MSP430_REGION_ prefix to enum values.
* config/msp430/msp430.c: Update use of enum values.
* config/msp430/msp430.md: Likewise.
* config/msp430/msp430.opt: Likewise.

Index: config/msp430/msp430-opts.h
===
--- config/msp430/msp430-opts.h	(revision 237527)
+++ config/msp430/msp430-opts.h	(working copy)
@@ -22,19 +22,19 @@
 
 enum msp430_hwmult_types
 {
-  NONE,
-  AUTO,
-  SMALL,
-  LARGE,
-  F5SERIES
+  MSP430_HWMULT_NONE,
+  MSP430_HWMULT_AUTO,
+  MSP430_HWMULT_SMALL,
+  MSP430_HWMULT_LARGE,
+  MSP430_HWMULT_F5SERIES
 };
 
 enum msp430_regions
 {
-  ANY,
-  EITHER,
-  LOWER,
-  UPPER
+  MSP430_REGION_ANY,
+  MSP430_REGION_EITHER,
+  MSP430_REGION_LOWER,
+  MSP430_REGION_UPPER
 };
 
 #endif
Index: config/msp430/msp430.c
===
--- config/msp430/msp430.c	(revision 237527)
+++ config/msp430/msp430.c	(working copy)
@@ -777,20 +777,21 @@
 			   target_mcu, xisa ? "430X" : "430", msp430x ? "430X" : "430");
 
 		if (msp430_mcu_data[i].hwmpy == 0
-		&& msp430_hwmult_type != AUTO
-		&& msp430_hwmult_type != NONE)
+		&& msp430_hwmult_type != MSP430_HWMULT_AUTO
+		&& msp430_hwmult_type != MSP430_HWMULT_NONE)
 		  warning (0, "MCU '%s' does not have hardware multiply support, but -mhwmult is set to %s",
 			   target_mcu,
-			   msp430_hwmult_type == SMALL ? "16-bit" : msp430_hwmult_type == LARGE ? "32-bit" : "f5series");
-		else if (msp430_hwmult_type == SMALL
+			   msp430_hwmult_type == MSP430_HWMULT_SMALL ? "16-bit"
+			   : msp430_hwmult_type == MSP430_HWMULT_LARGE ? "32-bit" : "f5series");
+		else if (msp430_hwmult_type == MSP430_HWMULT_SMALL
 		&& msp430_mcu_data[i].hwmpy != 1
 		&& msp430_mcu_data[i].hwmpy != 2 )
 		  warning (0, "MCU '%s' supports %s hardware multiply, but -mhwmult is set to 16-bit",
 			   target_mcu, hwmult_name (msp430_mcu_data[i].hwmpy));
-		else if (msp430_hwmult_type == LARGE && msp430_mcu_data[i].hwmpy != 4)
+		else if (msp430_hwmult_type == MSP430_HWMULT_LARGE && msp430_mcu_data[i].hwmpy != 4)
 		  warning (0, "MCU '%s' supports %s hardware multiply, but -mhwmult is set to 32-bit",
 			   target_mcu, hwmult_name (msp430_mcu_data[i].hwmpy));
-		else if (msp430_hwmult_type == F5SERIES && msp430_mcu_data[i].hwmpy != 8)
+		else if (msp430_hwmult_type == MSP430_HWMULT_F5SERIES && msp430_mcu_data[i].hwmpy != 8)
 		  warning (0, "MCU '%s' supports %s hardware multiply, but -mhwmult is set to f5series",
 			   target_mcu, hwmult_name (msp430_mcu_data[i].hwmpy));
 	  }
@@ -801,7 +802,7 @@
 
   if (i < 0)
 	{
-	  if (msp430_hwmult_type == AUTO)
+	  if (msp430_hwmult_type == MSP430_HWMULT_AUTO)
 	{
 	  if (msp430_warn_mcu)
 		{
@@ -815,7 +816,7 @@
 			 target_mcu);
 		}
 
-	  msp430_hwmult_type = NONE;
+	  msp430_hwmult_type = MSP430_HWMULT_NONE;
 	}
 	  else if (target_cpu == NULL)
 	{
@@ -833,15 +834,15 @@
 }
 
   /* The F5 series are all able to support the 430X ISA.  */
-  if (target_cpu == NULL && target_mcu == NULL && msp430_hwmult_type == F5SERIES)
+  if (target_cpu == NULL && target_mcu == NULL && msp430_hwmult_type == MSP430_HWMULT_F5SERIES)
 msp430x = true;
 
   if (TARGET_LARGE && !msp430x)
 error ("-mlarge requires a 430X-compatible -mmcu=");
 
-  if (msp430_code_region == UPPER && ! msp430x)
+  if (msp430_code_region == MSP430_REGION_UPPER && ! msp430x)
 error ("-mcode-region=upper requires 430X-compatible cpu");
-  if (msp430_data_region == UPPER && ! msp430x)
+  if (msp430_data_region == MSP430_REGION_UPPER && ! msp430x)
 error ("-mdata-region=upper requires 430X-compatible cpu");
 
   if (flag_exceptions || flag_non_call_exceptions
@@ -2166,24 +2167,24 @@
 
   if (TREE_CODE (decl) == FUNCTION_DECL)
 {
-  if (msp430_code_region == LOWER)
+  if (msp430_code_region == MSP430_REGION_LOWER)
 	return lower_prefix;
 
-  if (msp430_code_region == UPPER)
+  if (msp430_code_region == MSP430_REGION_UPPER)
 	return upper_prefix;
 
-  if (msp430_code_region ==

Re: [RX] Add support for atomic operations

[Nick Clifton]

Hi Oleg,

> Sorry, but my original patch was buggy.  There are two problems:

Thanks for your diligence in checking the patch.

> The attached patch fixes those issues.
> OK for trunk?
> 
> Cheers,
> Oleg
> 
> gcc/ChangeLog:
>   * config/rx/rx.md (FETCHOP_NO_MINUS): New code iterator.
>   (atomic__fetchsi): Extract minus operator into ...
>   (atomic_sub_fetchsi): ... this new pattern.
>   (mvtc): Add CC_REG clobber.

Approved - please apply.

Cheers

Re: RFA: Generate normal DWARF DW_LOC descriptors for non integer mode pointers

[Nick Clifton]

Hi Jeff,

>>> I may be missing something, but isn't it the transition to an FP
>>> relative address rather than a SP relative address that's the problem
>>> here?
>>
>> Yes, I believe so.
>>
>>> Where does that happen?

I think that it happens in dwarf2out.c:based_loc_descr()  which
detects the use of the frame pointer and works out that it is going 
to be eliminated to the stack pointer:

  /* We only use "frame base" when we're sure we're talking about the
 post-prologue local stack frame.  We do this by *not* running
 register elimination until this point, and recognizing the special
 argument pointer and soft frame pointer rtx's.  */
  if (reg == arg_pointer_rtx || reg == frame_pointer_rtx)
{
  rtx elim = (ira_use_lra_p
  ? lra_eliminate_regs (reg, VOIDmode, NULL_RTX)
  : eliminate_regs (reg, VOIDmode, NULL_RTX));

  if (elim != reg)
.

The problem, I believe, is that based_loc_descr() is only called
from mem_loc_descriptor when the mode of the rtl concerned is an
MODE_INT.  For example:

case REG:
  if (GET_MODE_CLASS (mode) != MODE_INT
 [...]
  else
  if (REGNO (rtl) < FIRST_PSEUDO_REGISTER)
mem_loc_result = based_loc_descr (rtl, 0, VAR_INIT_STATUS_INITIALIZED);

or, (this is another one that I found whilst investigating this 
problem further):

  case PLUS:
plus:
  if (is_based_loc (rtl)
  && (GET_MODE_SIZE (mode) <= DWARF2_ADDR_SIZE
  || XEXP (rtl, 0) == arg_pointer_rtx
  || XEXP (rtl, 0) == frame_pointer_rtx)
  && GET_MODE_CLASS (mode) == MODE_INT)
mem_loc_result = based_loc_descr (XEXP (rtl, 0),
  INTVAL (XEXP (rtl, 1)),
  VAR_INIT_STATUS_INITIALIZED);
  else


There are quite a few places in mem_loc_descriptor where the code checks
for the mode being in the MODE_INT class.  I am not exactly sure why.  I
think that it might be that the programmer thought that any expression that
does not involve integer based arithmetic cannot be expressed in DWARF CFA
notation and so would have to be handled specially.  If I am correct,
then it seems to me that the proper fix would be to use SCALAR_INT_MODE_P
instead.

I tried out the extended patch (attached) and it gave even better GDB 
results for the MSP430 and still no regressions (GCC or GDB) for MSP430 or
x86_64.

Is this enough justification ?

Cheers
  Nick



dwarf2out.c.patch.2
Description: Unix manual page

Commit: MSP430: Three small fixes

[Nick Clifton]

Hi Guys,

  I am checking in the patch below to fix three small problems with the
  MSP430 backend:

  Firstly interrupt handlers for the MSP430 cannot be static.  Static
  interrupt handlers can be optimized away since no control flow path
  can be found to use them.

  Secondly there no longer is a default linker script called msp430.md
  provided by libgloss.  Instead the default script built in to the
  linker should be used.  (Or better, a MCU specific linker script).

  Finally a failure in the gcc testsuite - gcc.dg/pr47201.c - was being
  caused because gcc could not find a way to extract the low 16 bits
  of a 20 bit pointer.  Adding a simple pattern to the machine
  description file fixes this.

Cheers
  Nick

gcc/ChangeLog
2016-05-25  Nick Clifton  <ni...@redhat.com>

* config/msp430/msp430.c (msp430_attr): Produce an error if a
static interrupt handler is detected.
* config/msp430/msp430.h (LIB_SPEC): Do not use msp430.ld as the
default linker script.
* config/msp430/msp430.md (movpsihi2_lo): New pattern for loading
the low part of a symbolic pointer.

Index: config/msp430/msp430.c
===
--- config/msp430/msp430.c  (revision 236703)
+++ config/msp430/msp430.c  (working copy)
@@ -1832,6 +1832,7 @@
 
   if (args != NULL)
 {
+  /* Only the interrupt attribute takes an argument.  */
   gcc_assert (TREE_NAME_EQ (name, ATTR_INTR));
 
   tree value = TREE_VALUE (args);
@@ -1878,6 +1879,9 @@
   if (TREE_CODE (TREE_TYPE (* node)) == FUNCTION_TYPE
  && ! VOID_TYPE_P (TREE_TYPE (TREE_TYPE (* node
message = "interrupt handlers must be void";
+
+  if (! TREE_PUBLIC (* node))
+   message = "interrupt handlers cannot be static";
 }
   else if (TREE_NAME_EQ (name, ATTR_REENT))
 {
Index: config/msp430/msp430.h
===
--- config/msp430/msp430.h  (revision 236703)
+++ config/msp430/msp430.h  (working copy)
@@ -98,7 +98,6 @@
 %{!msim:-lnosys}   \
 --end-group\
 %{!T*:%{!msim:%{mmcu=*:--script=%*.ld}}}   \
-%{!T*:%{!msim:%{!mmcu=*:%Tmsp430.ld}}} \
 %{!T*:%{msim:%{mlarge:%Tmsp430xl-sim.ld}%{!mlarge:%Tmsp430-sim.ld}}} \
 "
 
Index: config/msp430/msp430.md
===
--- config/msp430/msp430.md (revision 236703)
+++ config/msp430/msp430.md (working copy)
@@ -267,6 +267,14 @@
   "PUSH.W\t%H1 { PUSH.W\t%L1 { POPM.A #1, %0 ; Move reg-pair %L1:%H1 into 
pointer %0"
 )
 
+;; Produced when converting a pointer to an integer via a union, eg 
gcc.dg/pr47201.c.
+(define_insn "*movpsihi2_lo"
+  [(set (match_operand:HI 0 "register_operand" "=r")
+   (subreg:HI (match_operand:PSI 1 "msp430_symbol_operand" "i") 0))]
+  "msp430x"
+  "MOVA\t%1, %0"
+)
+
 ;;
 ;; Math
 

Re: RFA: Generate normal DWARF DW_LOC descriptors for non integer mode pointers

[Nick Clifton]

Hi Jeff,

>>   Currently dwarf2out.c:mem_loc_descriptor() has some special case
>>   code to handle the situation where an address is held in a register
>>   whose mode is not of type MODE_INT.  It generates a
>>   DW_OP_GNU_regval_type expression which may later on be converted into
>>   a frame pointer based expression.  This is a problem for targets which
>>   use a partial integer mode for their pointers (eg the msp430).  In
>>   such cases the conversion to a frame pointer based expression could
>>   be wrong if the frame pointer is not being used.

> I may be missing something, but isn't it the transition to an FP 
> relative address rather than a SP relative address that's the problem 
> here?

Yes, I believe so.

> Where does that happen?  

I did not track it down.  But whilst I was searching for the cause I came
across the code that is modified by the patch.  Reading the code it seemed
obvious to me that the special case for handling non INT_MODE register modes
was not intended for pointers, and when I tried out a small patch it worked.

> Is it possible we've got the wrong DECL_RTL or somesuch?

I don't think so.  I am not familiar with this code myself, but the dump from 
the dwarf2 pass shows:

  (insn 5 2 6 (set (mem/c:HI (plus:PSI (reg/f:PSI 1 R1)
(const_int 4 [0x4])) [1 c+0 S2 A16])
(const_int 5 [0x5])) 
/work/sources/binutils/current/gdb/testsuite/gdb.base/advance.c:41 12 {movhi}
 (nil))

which to me pretty clearly shows that "c" is being stored at R1+4.

Cheers
  Nick