[PATCHv2] gdb/fortran: Fix printing of logical true values for Flang

Message ID 20200302182152.12819-1-andrew.burgess@embecosm.com
State New, archived
Headers

Commit Message

Andrew Burgess March 2, 2020, 6:21 p.m. UTC
  Alok,

Thanks for looking into this.  I think that the best solution right
now will be to handle TYPE_CODE_BOOL in f_val_print rather than
modifying generic_val_print_bool.

The other possibility would be, I think, to add a new field to 'struct
language_defn' and use this in generic_val_print_bool instead of
comparing the value of current_lanuage directly, however, this isn't
the common approach, so I'd prefer to handle this case just like other
TYPE_CODE_* are handled for now.

I know that in places within GDB we do compare the value of
current_lanuage to the know language structures, but I'd like to move
us away from doing this.

I've gone ahead and added some tests too.

Let me know what you think of this.  If you're happy then I'll go
ahead and push this.

Thanks
Andrew

---

GDB is not able to print logical true values for Flang compiler.

Actual result:

  (gdb) p l
  $1 = 4294967295

Expected result:

  (gdb) p l
  $1 = .TRUE.

This is due to GDB expecting representation of true value being 1.
The Fortran standard doesnt specify how LOGICAL types are represented.
Different compilers use different non-zero values to represent logical
true. The gfortran compiler uses 1 to represent logical true and the
flang compiler uses -1. GDB should accept all the non-zero values as
true.

This is achieved by handling TYPE_CODE_BOOL in f_val_print and
printing any non-zero value as true.

gdb/ChangeLog:

	* f-valprint.c (f_val_print): Handle TYPE_CODE_BOOL, any non-zero
	value should be printed as true.

gdb/testsuite/ChangeLog:

	* gdb.fortran/logical.exp: Add tests that any non-zero value is
	printed as true.
---
 gdb/ChangeLog                         |  6 ++++++
 gdb/f-valprint.c                      | 25 ++++++++++++++++++++++++-
 gdb/testsuite/ChangeLog               |  5 +++++
 gdb/testsuite/gdb.fortran/logical.exp | 18 ++++++++++++++++++
 4 files changed, 53 insertions(+), 1 deletion(-)
  

Comments

Tom Tromey March 3, 2020, 4:31 p.m. UTC | #1
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Thanks for looking into this.  I think that the best solution right
Andrew> now will be to handle TYPE_CODE_BOOL in f_val_print rather than
Andrew> modifying generic_val_print_bool.

Agreed.

Andrew> The other possibility would be, I think, to add a new field to 'struct
Andrew> language_defn' and use this in generic_val_print_bool instead of
Andrew> comparing the value of current_lanuage directly, however, this isn't
Andrew> the common approach, so I'd prefer to handle this case just like other
Andrew> TYPE_CODE_* are handled for now.

It would be nice if we could get away from having separate printing code
for each language.  I don't know how practical this is though.

Andrew> I know that in places within GDB we do compare the value of
Andrew> current_lanuage to the know language structures, but I'd like to move
Andrew> us away from doing this.

Also agreed.

Tom
  
Andrew Burgess March 3, 2020, 5:24 p.m. UTC | #2
* Tom Tromey <tom@tromey.com> [2020-03-03 09:31:55 -0700]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> Thanks for looking into this.  I think that the best solution right
> Andrew> now will be to handle TYPE_CODE_BOOL in f_val_print rather than
> Andrew> modifying generic_val_print_bool.
> 
> Agreed.
> 
> Andrew> The other possibility would be, I think, to add a new field to 'struct
> Andrew> language_defn' and use this in generic_val_print_bool instead of
> Andrew> comparing the value of current_lanuage directly, however, this isn't
> Andrew> the common approach, so I'd prefer to handle this case just like other
> Andrew> TYPE_CODE_* are handled for now.
> 
> It would be nice if we could get away from having separate printing code
> for each language.  I don't know how practical this is though.

I am slowly working on converting the language structure into a true
class, my thinking is that this would make it much easier to add more
per-language functions, and specialise as needed.

In this case we would then remove the special handling for Fortran and
make the common code be something like:

      ....
      const gdb_byte *valaddr = value_contents_for_printing (original_value);

      val = unpack_long (type, valaddr + embedded_offset * unit_size);
      if (current_language->is_logical_false (val))
	fputs_filtered (decorations->false_name, stream);
      else if (current_language->is_logical_true (val))
	fputs_filtered (decorations->true_name, stream);
      else
	print_longest (stream, 'd', 0, val);
      ....

I think there's lots of places in the value printing (and maybe
evaluation) code where we copy blocks of code just to allow for small
specialisations, I'd hope we could start to recombine some of this
stuff...

Thanks,
Andrew



> 
> Andrew> I know that in places within GDB we do compare the value of
> Andrew> current_lanuage to the know language structures, but I'd like to move
> Andrew> us away from doing this.
> 
> Also agreed.
> 
> Tom
  

Patch

diff --git a/gdb/f-valprint.c b/gdb/f-valprint.c
index a25e6147322..0393ddfa8ab 100644
--- a/gdb/f-valprint.c
+++ b/gdb/f-valprint.c
@@ -357,6 +357,30 @@  f_val_print (struct type *type, int embedded_offset,
       fprintf_filtered (stream, " )");
       break;     
 
+    case TYPE_CODE_BOOL:
+      if (options->format || options->output_format)
+	{
+	  struct value_print_options opts = *options;
+	  opts.format = (options->format ? options->format
+			 : options->output_format);
+	  val_print_scalar_formatted (type, embedded_offset,
+				      original_value, &opts, 0, stream);
+	}
+      else
+	{
+	  int unit_size = gdbarch_addressable_memory_unit_size (gdbarch);
+	  LONGEST val
+	    = unpack_long (type, valaddr + embedded_offset * unit_size);
+	  /* The Fortran standard doesn't specify how logical types are
+	     represented.  Different compilers use different non zero
+	     values to represent logical true.  */
+	  if (val == 0)
+	    fputs_filtered (f_decorations.false_name, stream);
+	  else
+	    fputs_filtered (f_decorations.true_name, stream);
+	}
+      break;
+
     case TYPE_CODE_REF:
     case TYPE_CODE_FUNC:
     case TYPE_CODE_FLAGS:
@@ -366,7 +390,6 @@  f_val_print (struct type *type, int embedded_offset,
     case TYPE_CODE_RANGE:
     case TYPE_CODE_UNDEF:
     case TYPE_CODE_COMPLEX:
-    case TYPE_CODE_BOOL:
     case TYPE_CODE_CHAR:
     default:
       generic_val_print (type, embedded_offset, address,
diff --git a/gdb/testsuite/gdb.fortran/logical.exp b/gdb/testsuite/gdb.fortran/logical.exp
index f0028159e59..96e6f8f9559 100644
--- a/gdb/testsuite/gdb.fortran/logical.exp
+++ b/gdb/testsuite/gdb.fortran/logical.exp
@@ -33,3 +33,21 @@  gdb_test "p l1" " = \\.TRUE\\."
 gdb_test "p l2" " = \\.TRUE\\."
 gdb_test "p l4" " = \\.TRUE\\."
 gdb_test "p l8" " = \\.TRUE\\."
+
+# Different Fortran compilers use different values for logical true.
+# Check how GDB handles this by modifying the underlying value for our
+# logical variables and check they still print as true.
+foreach_with_prefix var { l l1 l2 l4 l8 } {
+    set len [get_integer_valueof "sizeof (${var})" "get sizeof ${var}"]
+    set addr [get_hexadecimal_valueof "&l" "get address of ${var}"]
+
+    for { set i 0 } { $i < $len } { incr i } {
+	with_test_prefix "byte $i" {
+	    gdb_test_no_output "set *((uint8_t *) ${addr}) = 0xff" \
+		"set contents of byte at offset $i"
+	    gdb_test "p l" " = \\.TRUE\\."
+	    incr addr
+	    set addr [format "0x%x" $addr]
+	}
+    }
+}