Make BLOCK_START and BLOCK_END into rvalues

Message ID 20200516151021.7080-1-tom@tromey.com
State New
Headers show
Series
  • Make BLOCK_START and BLOCK_END into rvalues
Related show

Commit Message

Tom Tromey May 16, 2020, 3:10 p.m.
This changes the BLOCK_START and BLOCK_END macros to expand to
rvalues, and then updates some code to assign to these fields
directly.

I considered adding setters here, but that seemed a bit needless,
given that these are "open" structures.  However, I can do that if
it's preferable.  Note that when the time comes to remove the
BLOCK_START and BLOCK_END macros, I think they will be replaced with
methods, because that will make it simpler to achieve objfile
independence for blocks.

This patch is one of a set of small cleanups related to
objfile-splitting that I've had kicking around for years.  I figured I
should start putting them in piecemeal rather than wait for the whole
series to be finished, as that seems very difficult.

gdb/ChangeLog
2020-05-16  Tom Tromey  <tom@tromey.com>

	* mips-tdep.c (mips_make_symbol_special): Update.
	* objfiles.c (objfile_relocate1): Update.
	* mdebugread.c (parse_symbol, mdebug_expand_psymtab)
	(sort_blocks): Update.
	* jit.c (finalize_symtab): Update.
	* buildsym.c (buildsym_compunit::finish_block_internal): Update.
	* block.h (BLOCK_START, BLOCK_END): Expand to rvalue.
---
 gdb/ChangeLog    | 10 ++++++++++
 gdb/block.h      |  4 ++--
 gdb/buildsym.c   | 10 +++++-----
 gdb/jit.c        |  8 ++++----
 gdb/mdebugread.c | 28 ++++++++++++++--------------
 gdb/mips-tdep.c  |  2 +-
 gdb/objfiles.c   |  4 ++--
 7 files changed, 38 insertions(+), 28 deletions(-)

-- 
2.17.2

Comments

Simon Marchi May 16, 2020, 4:15 p.m. | #1
On 2020-05-16 11:10 a.m., Tom Tromey wrote:
> This changes the BLOCK_START and BLOCK_END macros to expand to

> rvalues, and then updates some code to assign to these fields

> directly.

> 

> I considered adding setters here, but that seemed a bit needless,

> given that these are "open" structures.  However, I can do that if

> it's preferable.  Note that when the time comes to remove the

> BLOCK_START and BLOCK_END macros, I think they will be replaced with

> methods, because that will make it simpler to achieve objfile

> independence for blocks.


This is similar to what I'm doing to struct type, so it would be nice to agree
on the way forward for all these structures, and change them in a consistent
manner.

What I'm doing/considering for struct type is to add getters / setters for all
properties.  The fields themselves should really become private, but it's
probably not possible to do it right now, since the structures need to remain
POD.  I think we could still prefix them with `m_` while leaving them public,
doing so will make sure to catch any place that was accessing the field directly,
not using the macro.  And when writing new code, it will be semi-obvious by the
naming that you shouldn't access the field directly.

Also, the getter can't have the same name as the field, so in some cases that
would be another reason to rename the field.  Although in the case of this patch,
I would name the getters simply `start` and `end`, just like the macros are
named `..._START` and `..._END`, so there's no conflict (we could still rename
the fields to `m_start` and `m_end` for consistency).

Since you are changing the places that modify the field values, I'd suggest
adding the setter right away, it's a step in the right direction.

And I'd suggest adding the getter too, changing the BLOCK_START macro to use
it, it shouldn't be much more work.  And it will achieve the goal of making
the macro yield an rvalue.

If you want, I can look into removing the macros after that, like I do for
the type macros.

Simon
H.J. Lu via Gdb-patches May 16, 2020, 4:36 p.m. | #2
On Sat, May 16, 2020, 10:10 Tom Tromey <tom@tromey.com> wrote:

> diff --git a/gdb/buildsym.c b/gdb/buildsym.c

> index b9bcc33080a..b13857b0dda 100644

> --- a/gdb/buildsym.c

> +++ b/gdb/buildsym.c

> @@ -242,8 +242,8 @@ buildsym_compunit::finish_block_internal

>         }

>      }

>

> -  BLOCK_START (block) = start;

> -  BLOCK_END (block) = end;

> +  block->startaddr = start = start;

>


I think you only need one "start" here :)

>

>
Tom Tromey May 17, 2020, 2:05 p.m. | #3
Simon> This is similar to what I'm doing to struct type, so it would be nice to agree
Simon> on the way forward for all these structures, and change them in a consistent
Simon> manner.

Makes sense.

Simon> Since you are changing the places that modify the field values, I'd suggest
Simon> adding the setter right away, it's a step in the right direction.

Simon> And I'd suggest adding the getter too, changing the BLOCK_START macro to use
Simon> it, it shouldn't be much more work.  And it will achieve the goal of making
Simon> the macro yield an rvalue.

I wasn't sure whether setters and getters would provide any real value.
Aside from some maybe-future idea of changing the getter, in other
situations they are just wrappers for the field.

For struct type, maybe it makes more sense, because one (very-)
long-term idea there would be to split up struct main_type into an
inheritance hierarchy, so that each type can be more obviously
type-safe, and also carry just the data it needs.

I don't mind making the change -- it's easy enough to do.

Tom
Simon Marchi May 17, 2020, 2:40 p.m. | #4
On 2020-05-17 10:05 a.m., Tom Tromey wrote:
> Simon> This is similar to what I'm doing to struct type, so it would be nice to agree

> Simon> on the way forward for all these structures, and change them in a consistent

> Simon> manner.

> 

> Makes sense.

> 

> Simon> Since you are changing the places that modify the field values, I'd suggest

> Simon> adding the setter right away, it's a step in the right direction.

> 

> Simon> And I'd suggest adding the getter too, changing the BLOCK_START macro to use

> Simon> it, it shouldn't be much more work.  And it will achieve the goal of making

> Simon> the macro yield an rvalue.

> 

> I wasn't sure whether setters and getters would provide any real value.

> Aside from some maybe-future idea of changing the getter, in other

> situations they are just wrappers for the field.


It's true that simply wrapping a field with a getter / setter may not appear valuable.
I think it is somewhat valuable to be able to put a breakpoint on the setter.  If you
are wondering where some block with a given start address is created, you can put a
conditional breakpoint on the setter.  And to avoid the field being changed behind the
back of that setter, then that field must be private (ideally) and a getter becomes
necessary as well.

I also like getter / setters because they allow adding assertions to make sure things
are in a coherent state.

> For struct type, maybe it makes more sense, because one (very-)

> long-term idea there would be to split up struct main_type into an

> inheritance hierarchy, so that each type can be more obviously

> type-safe, and also carry just the data it needs.

> 

> I don't mind making the change -- it's easy enough to do.


As you wish :)

Simon

Patch

diff --git a/gdb/block.h b/gdb/block.h
index 50ab049f8e6..395f72d1bc9 100644
--- a/gdb/block.h
+++ b/gdb/block.h
@@ -139,8 +139,8 @@  struct global_block
   struct compunit_symtab *compunit_symtab;
 };
 
-#define BLOCK_START(bl)		(bl)->startaddr
-#define BLOCK_END(bl)		(bl)->endaddr
+#define BLOCK_START(bl)		(((bl)->startaddr) + 0)
+#define BLOCK_END(bl)		(((bl)->endaddr) + 0)
 #define BLOCK_FUNCTION(bl)	(bl)->function
 #define BLOCK_SUPERBLOCK(bl)	(bl)->superblock
 #define BLOCK_MULTIDICT(bl)	(bl)->multidict
diff --git a/gdb/buildsym.c b/gdb/buildsym.c
index b9bcc33080a..b13857b0dda 100644
--- a/gdb/buildsym.c
+++ b/gdb/buildsym.c
@@ -242,8 +242,8 @@  buildsym_compunit::finish_block_internal
 	}
     }
 
-  BLOCK_START (block) = start;
-  BLOCK_END (block) = end;
+  block->startaddr = start = start;
+  block->endaddr = end;
 
   /* Put the block in as the value of the symbol that names it.  */
 
@@ -329,7 +329,7 @@  buildsym_compunit::finish_block_internal
 		     paddress (gdbarch, BLOCK_START (block)));
 	}
       /* Better than nothing.  */
-      BLOCK_END (block) = BLOCK_START (block);
+      block->endaddr = BLOCK_START (block);
     }
 
   /* Install this block as the superblock of all blocks made since the
@@ -368,9 +368,9 @@  buildsym_compunit::finish_block_internal
 			     paddress (gdbarch, BLOCK_END (block)));
 		}
 	      if (BLOCK_START (pblock->block) < BLOCK_START (block))
-		BLOCK_START (pblock->block) = BLOCK_START (block);
+		pblock->block->startaddr = BLOCK_START (block);
 	      if (BLOCK_END (pblock->block) > BLOCK_END (block))
-		BLOCK_END (pblock->block) = BLOCK_END (block);
+		pblock->block->endaddr = BLOCK_END (block);
 	    }
 	  BLOCK_SUPERBLOCK (pblock->block) = block;
 	}
diff --git a/gdb/jit.c b/gdb/jit.c
index 1b5ef46469e..c099d23e5e2 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -664,8 +664,8 @@  finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
       BLOCK_MULTIDICT (new_block)
 	= mdict_create_linear (&objfile->objfile_obstack, NULL);
       /* The address range.  */
-      BLOCK_START (new_block) = (CORE_ADDR) gdb_block_iter.begin;
-      BLOCK_END (new_block) = (CORE_ADDR) gdb_block_iter.end;
+      new_block->startaddr = (CORE_ADDR) gdb_block_iter.begin;
+      new_block->endaddr = (CORE_ADDR) gdb_block_iter.end;
 
       /* The name.  */
       SYMBOL_DOMAIN (block_name) = VAR_DOMAIN;
@@ -704,8 +704,8 @@  finalize_symtab (struct gdb_symtab *stab, struct objfile *objfile)
       BLOCK_SUPERBLOCK (new_block) = block_iter;
       block_iter = new_block;
 
-      BLOCK_START (new_block) = (CORE_ADDR) begin;
-      BLOCK_END (new_block) = (CORE_ADDR) end;
+      new_block->startaddr = (CORE_ADDR) begin;
+      new_block->endaddr = (CORE_ADDR) end;
 
       BLOCKVECTOR_BLOCK (bv, i) = new_block;
 
diff --git a/gdb/mdebugread.c b/gdb/mdebugread.c
index ba53512636e..2a3f9492897 100644
--- a/gdb/mdebugread.c
+++ b/gdb/mdebugread.c
@@ -792,7 +792,7 @@  parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
       b = new_block (FUNCTION_BLOCK, s->language ());
       SYMBOL_BLOCK_VALUE (s) = b;
       BLOCK_FUNCTION (b) = s;
-      BLOCK_START (b) = BLOCK_END (b) = sh->value;
+      b->startaddr = b->endaddr = sh->value;
       BLOCK_SUPERBLOCK (b) = top_stack->cur_block;
       add_block (b, top_stack->cur_st);
 
@@ -1121,7 +1121,7 @@  parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
 
       top_stack->blocktype = stBlock;
       b = new_block (NON_FUNCTION_BLOCK, psymtab_language);
-      BLOCK_START (b) = sh->value + top_stack->procadr;
+      b->startaddr = sh->value + top_stack->procadr;
       BLOCK_SUPERBLOCK (b) = top_stack->cur_block;
       top_stack->cur_block = b;
       add_block (b, top_stack->cur_st);
@@ -1145,7 +1145,7 @@  parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
 	  struct type *ftype = top_stack->cur_type;
 	  int i;
 
-	  BLOCK_END (top_stack->cur_block) += sh->value;	/* size */
+	  top_stack->cur_block->endaddr += sh->value;	/* size */
 
 	  /* Make up special symbol to contain procedure specific info.  */
 	  s = new_symbol (MDEBUG_EFI_SYMBOL_NAME);
@@ -1169,8 +1169,8 @@  parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
 		  && BLOCK_START (b_bad) == top_stack->procadr
 		  && BLOCK_END (b_bad) == top_stack->procadr)
 		{
-		  BLOCK_START (b_bad) = BLOCK_START (cblock);
-		  BLOCK_END (b_bad) = BLOCK_END (cblock);
+		  b_bad->startaddr = BLOCK_START (cblock);
+		  b_bad->endaddr = BLOCK_END (cblock);
 		}
 	    }
 
@@ -1211,7 +1211,7 @@  parse_symbol (SYMR *sh, union aux_ext *ax, char *ext_sh, int bigend,
 	  /* End of (code) block.  The value of the symbol is the
 	     displacement from the procedure`s start address of the
 	     end of this block.  */
-	  BLOCK_END (top_stack->cur_block) = sh->value + top_stack->procadr;
+	  top_stack->cur_block->endaddr = sh->value + top_stack->procadr;
 	}
       else if (sh->sc == scText && top_stack->blocktype == stNil)
 	{
@@ -4081,8 +4081,8 @@  mdebug_expand_psymtab (legacy_psymtab *pst, struct objfile *objfile)
       top_stack->cur_st = COMPUNIT_FILETABS (cust);
       top_stack->cur_block
 	= BLOCKVECTOR_BLOCK (COMPUNIT_BLOCKVECTOR (cust), STATIC_BLOCK);
-      BLOCK_START (top_stack->cur_block) = pst->text_low (objfile);
-      BLOCK_END (top_stack->cur_block) = 0;
+      top_stack->cur_block->startaddr = pst->text_low (objfile);
+      top_stack->cur_block->endaddr = 0;
       top_stack->blocktype = stFile;
       top_stack->cur_type = 0;
       top_stack->procadr = 0;
@@ -4555,9 +4555,9 @@  sort_blocks (struct symtab *s)
     {
       /* Cosmetic */
       if (BLOCK_END (BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK)) == 0)
-	BLOCK_START (BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK)) = 0;
+	BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK)->startaddr = 0;
       if (BLOCK_END (BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK)) == 0)
-	BLOCK_START (BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK)) = 0;
+	BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK)->endaddr = 0;
       return;
     }
   /*
@@ -4578,15 +4578,15 @@  sort_blocks (struct symtab *s)
     for (i = FIRST_LOCAL_BLOCK; i < j; i++)
       if (high < BLOCK_END (BLOCKVECTOR_BLOCK (bv, i)))
 	high = BLOCK_END (BLOCKVECTOR_BLOCK (bv, i));
-    BLOCK_END (BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK)) = high;
+    BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK)->endaddr = high;
   }
 
-  BLOCK_START (BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK)) =
+  BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK)->startaddr =
     BLOCK_START (BLOCKVECTOR_BLOCK (bv, FIRST_LOCAL_BLOCK));
 
-  BLOCK_START (BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK)) =
+  BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK)->startaddr =
     BLOCK_START (BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK));
-  BLOCK_END (BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK)) =
+  BLOCKVECTOR_BLOCK (bv, STATIC_BLOCK)->endaddr =
     BLOCK_END (BLOCKVECTOR_BLOCK (bv, GLOBAL_BLOCK));
 }
 
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index df59f416b8e..399b5c0c8d7 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -473,7 +473,7 @@  mips_make_symbol_special (struct symbol *sym, struct objfile *objfile)
       msym = lookup_minimal_symbol_by_pc (compact_block_start);
       if (msym.minsym && !msymbol_is_mips (msym.minsym))
 	{
-	  BLOCK_START (block) = compact_block_start;
+	  block->startaddr = compact_block_start;
 	}
     }
 }
diff --git a/gdb/objfiles.c b/gdb/objfiles.c
index 3aa7973e0d5..a1c6a44145a 100644
--- a/gdb/objfiles.c
+++ b/gdb/objfiles.c
@@ -690,8 +690,8 @@  objfile_relocate1 (struct objfile *objfile,
 	    struct mdict_iterator miter;
 
 	    b = BLOCKVECTOR_BLOCK (bv, i);
-	    BLOCK_START (b) += delta[block_line_section];
-	    BLOCK_END (b) += delta[block_line_section];
+	    b->startaddr += delta[block_line_section];
+	    b->endaddr += delta[block_line_section];
 
 	    if (BLOCK_RANGES (b) != nullptr)
 	      for (int j = 0; j < BLOCK_NRANGES (b); j++)