[2/9] binutils: Be more forgiving of targets with large numbers of registers

Message ID 006f9651afe537a624626fe187b9b5e0ba7d69b9.1574423265.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • DWARF Register Number Handling, Including RISC-V CSRs
Related show

Commit Message

Andrew Burgess Nov. 22, 2019, 12:10 p.m.
Currently if a target has a large ( > 1024 ) number of registers then
we get a warning when dumping the DWARF whenever a register over the
1024 limit is referenced, this occurs in dwarf.c:frame_need_space.

This check was initially introduced to guard against corrupted DWARF
referencing stupidly large numbers of registers.

The frame_need_space function already has a check in place so that, if
a target specifies a set of known DWARF register names then we must
only reference a register within this set, it is only after this check
that we check for the 1024 limit.

What this means is that if a target DOES NOT define a set of known
register names and if we reference more than 1024 registers
frame_need_space will give a warning.

If a target DOES define a set of known registers and there are more
than 1024 defined registers, and we try to reference a register beyond
1024 we will again get an error.

This second case feels wrong to me.  My thinking is that if a target
defines a set of registers then it is not unreasonable to assume the
tools can cope with that number of registers.  And so, if the target
defines 2000 named DWARF registers, frame_need_space should allow
access to all of these registers.

If a target does not define a set of named registers then the 1024
limit should remain.  This is pretty arbitrary, but we do need to have
some limit in place I think, so for now that seems as good as any.

This is an entirely theoretical fix - there are no targets that define
such large numbers of registers, but while experimenting with adding
support for RISC-V CSRs I ran into this issue and felt like it was a
good improvement.

binutils/ChangeLog:

	* dwarf.c (frame_need_space): Compare dwarf_regnames_count against
	0, and only warn about large numbers of registers if the number is
	more than the dwarf_regnames_count.

Change-Id: Ifac1a999ff0677676e81ee373c4c044b6a700827
---
 binutils/ChangeLog | 6 ++++++
 binutils/dwarf.c   | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

-- 
2.14.5

Comments

Palmer Dabbelt via binutils Nov. 22, 2019, 9:56 p.m. | #1
On Fri, 22 Nov 2019 04:10:26 PST (-0800), andrew.burgess@embecosm.com wrote:
> Currently if a target has a large ( > 1024 ) number of registers then

> we get a warning when dumping the DWARF whenever a register over the

> 1024 limit is referenced, this occurs in dwarf.c:frame_need_space.

>

> This check was initially introduced to guard against corrupted DWARF

> referencing stupidly large numbers of registers.

>

> The frame_need_space function already has a check in place so that, if

> a target specifies a set of known DWARF register names then we must

> only reference a register within this set, it is only after this check

> that we check for the 1024 limit.

>

> What this means is that if a target DOES NOT define a set of known

> register names and if we reference more than 1024 registers

> frame_need_space will give a warning.

>

> If a target DOES define a set of known registers and there are more

> than 1024 defined registers, and we try to reference a register beyond

> 1024 we will again get an error.

>

> This second case feels wrong to me.  My thinking is that if a target

> defines a set of registers then it is not unreasonable to assume the

> tools can cope with that number of registers.  And so, if the target

> defines 2000 named DWARF registers, frame_need_space should allow

> access to all of these registers.

>

> If a target does not define a set of named registers then the 1024

> limit should remain.  This is pretty arbitrary, but we do need to have

> some limit in place I think, so for now that seems as good as any.

>

> This is an entirely theoretical fix - there are no targets that define

> such large numbers of registers, but while experimenting with adding

> support for RISC-V CSRs I ran into this issue and felt like it was a

> good improvement.

>

> binutils/ChangeLog:

>

> 	* dwarf.c (frame_need_space): Compare dwarf_regnames_count against

> 	0, and only warn about large numbers of registers if the number is

> 	more than the dwarf_regnames_count.

>

> Change-Id: Ifac1a999ff0677676e81ee373c4c044b6a700827

> ---

>  binutils/ChangeLog | 6 ++++++

>  binutils/dwarf.c   | 4 ++--

>  2 files changed, 8 insertions(+), 2 deletions(-)

>

> diff --git a/binutils/dwarf.c b/binutils/dwarf.c

> index 2fe469f0603..62f2817d183 100644

> --- a/binutils/dwarf.c

> +++ b/binutils/dwarf.c

> @@ -7378,7 +7378,7 @@ frame_need_space (Frame_Chunk *fc, unsigned int reg)

>    if (reg < (unsigned int) fc->ncols)

>      return 0;

>

> -  if (dwarf_regnames_count

> +  if (dwarf_regnames_count > 0

>        && reg > dwarf_regnames_count)

>      return -1;


If I understand correctly, this doesn't actually change any behavior because
dwarf_regnames_count is never negative.  I'm fine with either way, though.

> @@ -7389,7 +7389,7 @@ frame_need_space (Frame_Chunk *fc, unsigned int reg)

>      return -1;

>

>    /* PR 17512: file: 2844a11d.  */

> -  if (fc->ncols > 1024)

> +  if (fc->ncols > 1024 && dwarf_regnames_count == 0)

>      {

>        error (_("Unfeasibly large register number: %u\n"), reg);

>        fc->ncols = 0;


Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>

Patch

diff --git a/binutils/dwarf.c b/binutils/dwarf.c
index 2fe469f0603..62f2817d183 100644
--- a/binutils/dwarf.c
+++ b/binutils/dwarf.c
@@ -7378,7 +7378,7 @@  frame_need_space (Frame_Chunk *fc, unsigned int reg)
   if (reg < (unsigned int) fc->ncols)
     return 0;
 
-  if (dwarf_regnames_count
+  if (dwarf_regnames_count > 0
       && reg > dwarf_regnames_count)
     return -1;
 
@@ -7389,7 +7389,7 @@  frame_need_space (Frame_Chunk *fc, unsigned int reg)
     return -1;
 
   /* PR 17512: file: 2844a11d.  */
-  if (fc->ncols > 1024)
+  if (fc->ncols > 1024 && dwarf_regnames_count == 0)
     {
       error (_("Unfeasibly large register number: %u\n"), reg);
       fc->ncols = 0;