[PATCH] default_type_align: Use type_length_units

[PATCH] default_type_align: Use type_length_units

[Simon Marchi]

The type alignment value is returned in 8-bit-bytes instead of target
memory addressable units.  For example, on a target with 16-bit-bytes
where sizeof(int) == 1 (one addressable unit), alignof(int) currently
returns 2.  After, this patch, it returns 1.

gdb/ChangeLog:

	* arch-utils.c (default_type_align): Use type_length_units.
---
 gdb/arch-utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 76bc16f..a65f01f 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -994,7 +994,7 @@ default_in_indirect_branch_thunk (gdbarch *gdbarch, CORE_ADDR pc)
 ULONGEST
 default_type_align (struct gdbarch *gdbarch, struct type *type)
 {
-  return TYPE_LENGTH (check_typedef (type));
+  return type_length_units (check_typedef (type));
 }
 
 void
-- 
2.7.4

Re: [PATCH] default_type_align: Use type_length_units

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@ericsson.com> writes:

Simon> The type alignment value is returned in 8-bit-bytes instead of target
Simon> memory addressable units.  For example, on a target with 16-bit-bytes
Simon> where sizeof(int) == 1 (one addressable unit), alignof(int) currently
Simon> returns 2.  After, this patch, it returns 1.

Simon> 	* arch-utils.c (default_type_align): Use type_length_units.

This certainly is correct according to the comment in gdbtypes.h:

  /* * Return the alignment of the type in target addressable memory
     units.

However, I was curious to know what DWARF specifies, to see whether
dwarf2read was doing this as well.  But DWARF seems pretty silent on
this topic:

  A debugging information entry may have a DW_AT_alignment attribute
  whose value of class constant is a positive, non-zero, integer
  describing the alignment of the entity.

So maybe this is depending on what your compiler does?  I think I did
not really consider this issue too deeply when writing the alignment
support :(

Another possible issue is that gdbtypes.c:type_align uses TYPE_LENGTH in
one spot but I suppose it ought to use type_length_units if this patch
goes in.

Tom

Re: [PATCH] default_type_align: Use type_length_units

[Simon Marchi]

On 2018-06-14 12:59, Tom Tromey wrote:
> This certainly is correct according to the comment in gdbtypes.h:
> 
>   /* * Return the alignment of the type in target addressable memory
>      units.
> 
> However, I was curious to know what DWARF specifies, to see whether
> dwarf2read was doing this as well.  But DWARF seems pretty silent on
> this topic:
> 
>   A debugging information entry may have a DW_AT_alignment attribute
>   whose value of class constant is a positive, non-zero, integer
>   describing the alignment of the entity.
> 
> So maybe this is depending on what your compiler does?  I think I did
> not really consider this issue too deeply when writing the alignment
> support :(

 From what I remember, nothing is well defined in DWARF regarding 
non-8-bit-bytes.  In the past, I have contributed fixes for such 
architectures based on how things work with our (Ericsson) internal 
compiler, and nobody complained so far.  There is a BoF at Cauldron 
though to talk about how to make the non-8-bit-bytes support more 
"official" and testable [1], if you're interested!

Our compiler doesn't emit DW_AT_alignment AFAIK, so I don't know what it 
would emit.  But it would make sense if it was in addressable memory 
units, just like type sizes.

> Another possible issue is that gdbtypes.c:type_align uses TYPE_LENGTH 
> in
> one spot but I suppose it ought to use type_length_units if this patch
> goes in.

This use seems related to C++, and our compiler is C only, so I can't 
test that.  But it would make sense, I can include that change if you 
prefer.

Simon

[1] 
https://gcc.gnu.org/wiki/cauldron2018#Non-8-bit-byte_architecture_support_BoF

Re: [PATCH] default_type_align: Use type_length_units

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> This use seems related to C++, and our compiler is C only, so I can't
Simon> test that.  But it would make sense, I can include that change if you
Simon> prefer.

Yeah, I think it would be best to either change both or change neither
(and update the comment).

Tom

Re: [PATCH] default_type_align: Use type_length_units

[Simon Marchi]

On 2018-06-14 03:50 PM, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
> Simon> This use seems related to C++, and our compiler is C only, so I can't
> Simon> test that.  But it would make sense, I can include that change if you
> Simon> prefer.
> 
> Yeah, I think it would be best to either change both or change neither
> (and update the comment).

Here's what I pushed.  However, I parsed the "and update the comment" of your reply
after pushing.  Which comment did you want me to update?

Simon


From 55c748a1c01f33cec0f762969db1bae457e8bcca Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Thu, 14 Jun 2018 18:23:39 -0400
Subject: [PATCH] type alignment: Use type_length_units

The type alignment value is returned in 8-bit-bytes instead of target
memory addressable units.  For example, on a target with 16-bit-bytes
where sizeof(int) == 1 (one addressable unit), alignof(int) currently
returns 2.  After, this patch, it returns 1.

gdb/ChangeLog:

	* arch-utils.c (default_type_align): Use type_length_units.
	* gdbtypes.c (type_align): Use type_length_units.
---
 gdb/ChangeLog    | 5 +++++
 gdb/arch-utils.c | 2 +-
 gdb/gdbtypes.c   | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 60766ca77758..aba9a1a1b1f3 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2018-06-14  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* arch-utils.c (default_type_align): Use type_length_units.
+	* gdbtypes.c (type_align): Use type_length_units.
+
 2018-06-14  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

 	* cli/cli-script.c (_initialize_cli_script): Fix online documentation
diff --git a/gdb/arch-utils.c b/gdb/arch-utils.c
index 76bc16f37500..a65f01f83368 100644
--- a/gdb/arch-utils.c
+++ b/gdb/arch-utils.c
@@ -994,7 +994,7 @@ default_in_indirect_branch_thunk (gdbarch *gdbarch, CORE_ADDR pc)
 ULONGEST
 default_type_align (struct gdbarch *gdbarch, struct type *type)
 {
-  return TYPE_LENGTH (check_typedef (type));
+  return type_length_units (check_typedef (type));
 }

 void
diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index fdd28c0886e3..65b1211ec492 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -3072,7 +3072,7 @@ type_align (struct type *type)

     case TYPE_CODE_METHODPTR:
     case TYPE_CODE_MEMBERPTR:
-      align = TYPE_LENGTH (type);
+      align = type_length_units (type);
       break;

     case TYPE_CODE_VOID:
-- 
2.17.1

Re: [PATCH] default_type_align: Use type_length_units

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

Simon> Here's what I pushed.  However, I parsed the "and update the comment" of your reply
Simon> after pushing.  Which comment did you want me to update?

Sorry that was unclear -- I meant if you decided not to use
type_length_units, then update the comment in gdbtypes.h to follow.
But, since you did do the type_length_units change, that's not relevant.

Thanks for fixing this btw.

Tom