Add GDB prettyprinters for a few more MLIR types.

[csigg]

Removing unnecessary #includes.

[csigg]

Add missing newline at end of file.

[csigg]

Hi David, is it OK if I send these changes your way for review? There is maybe one or two more after that for now.

This revision primarily adds some helpers to skip printing wrapper types through inheritance or composition.

Thanks!

[dblaikie]

In D87159#2256991, @csigg wrote:

Hi David, is it OK if I send these changes your way for review? There is maybe one or two more after that for now.

This revision primarily adds some helpers to skip printing wrapper types through inheritance or composition.

Thanks!

Sure - I don't work on MLIR and have only written a few basic pretty printers myself - but happy to do what I can.

mlir/utils/gdb-scripts/prettyprinters.py

37–44

Sorry, I'm not understanding this comment - could you explain this infrastructure in different words/more detail?

[csigg]

Clarify documentation.

[csigg]

mlir/utils/gdb-scripts/prettyprinters.py

37–44

I hope this is clearer now.

[dblaikie]

mlir/utils/gdb-scripts/prettyprinters.py

37–44

Thanks for updating the comment, best way to improve this!

Hmm - so there's no existing infrastructure for querying gdb's printer for a type T that would behave the same way if that T were being printed directly by gdb?

eg: the SmallVector<T> printer should have something like this for some cases? (which I don't think it has) like if I just wrote "struct t1 { ... };" and had SmallVector<T> then printing the small vector wouldn't print out the t1 objects the same way as t1 objects not in the small vector would be printed? (perhaps an example or two would be good?)

[csigg]

mlir/utils/gdb-scripts/prettyprinters.py

37–44

The SmallVector<T> has it's own printer which returns the Ts as children. If there is no custom visualizer for T, GDB will print the *children* as raw.

This is different from the case of a printer factory for a type containing a single T.
A factory that wants to skip the container/wrapper type and print T directly can use this function to create a printer for T.

'get_first_member_printer' and 'get_first_base_printer' below use this function. Are those good enough as 'an example or two'?

[dblaikie]

mlir/utils/gdb-scripts/prettyprinters.py

37–44

The SmallVector<T> has it's own printer which returns the Ts as children. If there is no custom visualizer for T, GDB will print the *children* as raw.

Ah, so the same distinction applies to Optional, for instance? Since its value is modeled as a child it doesn't need to recurse into/reimplement struct printing?

Could that approach be taken here instead - so we don't have to reimplement GDB's generic struct printer/lookup printers? I think it'd be nice not to reimplement GDB's default printing behavior - it may be improved/modified/etc and we wouldn't benefit from those enhancements.

'get_first_member_printer' and 'get_first_base_printer' below use this function. Are those good enough as 'an example or two'?

Sorry, I meant more in terms of "using get_default_or_struct_printer this is the gdb behavior we can observer <example 1>, without it this is the (undesirable) behavior <example 2>" - showing the result of the pretty printer with/without this feature.

[csigg]

Removing StructPrinter and get_default_or_struct_printer, add the remaining printers in a single revision.

[csigg]

mlir/utils/gdb-scripts/prettyprinters.py

37–44

Could that approach be taken here instead

Yes, that's an option. You just can no longer skip that the value is a wrapped type. For Optional that might not be desired.

I wanted to have the printers reduce the amount of noise from wrapper types. For example, the OpaqueLoc from the latest revision now prints:

(gdb) p OpaqueLoc
$17 = {
  impl = {
    <mlir::Attribute> = {
      cast<mlir::OpaqueLoc> = {
        <mlir::detail::StorageUserBase<mlir::OpaqueLoc, mlir::LocationAttr, mlir::detail::OpaqueLocationStorage, mlir::detail::AttributeUniquer>> = {
          impl = {
            <mlir::AttributeStorage> = "mlir::OpaqueLoc", 
            members of mlir::detail::OpaqueLocationStorage:
            underlyingLocation = 9,
            typeID = {
              storage = 0x5555558c93f0 <mlir::TypeID::get<unsigned long>()::instance>
            },
            fallbackLocation = {
              impl = {
                <mlir::Attribute> = {
                  cast<mlir::UnknownLoc> = {
                    <mlir::detail::StorageUserBase<mlir::UnknownLoc, mlir::LocationAttr, mlir::AttributeStorage, mlir::detail::AttributeUniquer>> = {
                      impl = "mlir::UnknownLoc"
                    }, <No data fields>}
                }, <No data fields>}
            }
          }
        }, <No data fields>}
    }, <No data fields>}
}

before, it would have been

(gdb) p OpaqueLoc
$17 = {
    <mlir::AttributeStorage> = "mlir::OpaqueLoc", 
    underlyingLocation = 9,
    typeID = {
      storage = 0x5555558c93f0 <mlir::TypeID::get<unsigned long>()::instance>
    },
    fallbackLocation = "mlir::UnknownLoc"
}

But let's go with the former, I would prefer to finally wrap this up.

[csigg]

Including change to print mlir::Identifier, which had to be reverted in
change rG485e6db87293.

I haven't figured out how to run the test conditionally, feel free to
hold off with the review.

[dblaikie]

No worries- you can do it the other way with the fallback/mimic implementation if you like.

Trying to understand this all better to help review/guide the implementations. Appreciate the contributions and help understanding it all.

[dblaikie]

In D87159#2299106, @csigg wrote:

Including change to print mlir::Identifier, which had to be reverted in
change rG485e6db87293.

I haven't figured out how to run the test conditionally, feel free to
hold off with the review.

Ah, fair enough. Can hold off on committing this & happy to review any changes needed to this once the precursor patch is addressed.

[csigg]

Skip mlir-support in debuginfo-tests when MLIR project is not enabled.

Fix llvm-support test to work with RelWithDebInfo.

[jpienaar]

Thanks for doing this!

debuginfo-tests/llvm-prettyprinters/gdb/llvm-support.gdb

1 ↗

(On Diff #294952)

Left over from debugging?

[csigg]

debuginfo-tests/llvm-prettyprinters/gdb/llvm-support.gdb

1 ↗

(On Diff #294952)

Yes, but intentionally. The amount of context that FileCheck gives without dumping the input is rarely enough. But I don't mind reverting it (and adjusting mlir-support.gdb).

[mehdi_amini]

debuginfo-tests/llvm-prettyprinters/gdb/llvm-support.gdb

1 ↗

(On Diff #294952)

I expect this to be the default for FileCheck for a while now, where do you see a difference with this?

[csigg]

Remove --dump-input=fail.

[csigg]

debuginfo-tests/llvm-prettyprinters/gdb/llvm-support.gdb

1 ↗

(On Diff #294952)

Indeed, this was cargo cult.

[MaskRay]

In a clean build, debuginfo-tests/llvm-prettyprinters/gdb/mlir-support.cpp (check-debuginfo) fails to compile for me fatal error: 'mlir/IR/BuiltinTypes.h.inc' file not found

I think it is because -I $build/tools/mlir/include is not added but I cannot figure out how to add this include dir.

[mehdi_amini]

In D87159#2483665, @MaskRay wrote:

In a clean build, debuginfo-tests/llvm-prettyprinters/gdb/mlir-support.cpp (check-debuginfo) fails to compile for me fatal error: 'mlir/IR/BuiltinTypes.h.inc' file not found

I think it is because -I $build/tools/mlir/include is not added but I cannot figure out how to add this include dir.

Isn't this more an issue of CMake dependency? The build of mlir-support.cpp does not depends on generating mlir/IR/BuiltinTypes.h.inc?
If it was a missing include path, wouldn't it always fail and not only sometimes?

[MaskRay]

In D87159#2485113, @mehdi_amini wrote:

In D87159#2483665, @MaskRay wrote:

In a clean build, debuginfo-tests/llvm-prettyprinters/gdb/mlir-support.cpp (check-debuginfo) fails to compile for me fatal error: 'mlir/IR/BuiltinTypes.h.inc' file not found

I think it is because -I $build/tools/mlir/include is not added but I cannot figure out how to add this include dir.

Isn't this more an issue of CMake dependency? The build of mlir-support.cpp does not depends on generating mlir/IR/BuiltinTypes.h.inc?
If it was a missing include path, wouldn't it always fail and not only sometimes?

Yes, a CMake issue. add_dependencies(..., MLIRBuiltinTypesIncGen) does not work because that dependency does not add -I $build/tools/mlir/include (I don't know which rule adds the -I...)
Yes, it likely always fails. Very few people and build bots build check-debuginfo, though.

[mehdi_amini]

Fixed in 476db17dcb64ef3ec6e247f4b1c673b57f61a367 and 9e1aaa9943b814c22ae03f4abb3171dac8062801

debuginfo-tests/llvm-prettyprinters/gdb/mlir-support.cpp

 #include "mlir/IR/Identifier.h"
 #include "mlir/IR/MLIRContext.h"
+#include "mlir/IR/OperationSupport.h"
 
 mlir::MLIRContext Context;
 
 auto Identifier = mlir::Identifier::get("foo", &Context);
+mlir::OperationName OperationName("FooOp", &Context);
+mlir::Value Value({reinterpret_cast<void *>(0x8),
+                   mlir::Value::Kind::TrailingOpResult});
 
 int main() { return 0; }

debuginfo-tests/llvm-prettyprinters/gdb/mlir-support.gdb

 # RUN: gdb -q -batch -n -iex 'source %mlir_src_root/utils/gdb-scripts/prettyprinters.py' -x %s %llvm_tools_dir/check-gdb-mlir-support | FileCheck %s --dump-input=fail
 # REQUIRES: debug-info
 
 break main
 run
 
 # CHECK: "foo"
 p Identifier
+
+# CHECK: "FooOp"
+p OperationName
+
+# CHECK: pointer = 0xa0,
+# CHECK: value = mlir::Value::Kind::TrailingOpResult
+p Value
 No newline at end of file

mlir/utils/gdb-scripts/prettyprinters.py

 """GDB pretty printers for MLIR types."""
 
 import gdb.printing
 
+
 class IdentifierPrinter:
   """Prints an mlir::Identifier instance."""
 
   def __init__(self, val):
     self.entry = val['entry']
 
   def to_string(self):
-    ptr = (self.entry + 1).cast(gdb.lookup_type('char').pointer());
+    ptr = (self.entry + 1).cast(gdb.lookup_type('char').pointer())
     return ptr.string(length=self.entry['keyLength'])
 
   def display_hint(self):
     return 'string'
 
+
+class StructPrinter:
+  """Prints bases of a struct and its fields.
+
+  This mimics GDB's printing of instances without a custom printer.
+  """
+
+  def __init__(self, val):
+    self.val = val
+
+  def children(self):
+    for field in self.val.type.fields():
+      if field.is_base_class:
+        yield ('<%s>' % field.name, self.val.cast(field.type))
+      else:
+        yield (field.name, self.val[field.name])
+
+
+def get_default_or_struct_printer(val):
+  """Returns gdb.default_visualizer(val) or StructPrinter(val).
+
+  A printer factory which forwards to a single instance field may want to always
+  return a valid printer. Otherwise, GDB will print the instance as raw,
+  defeating the purpose of forwarding. This function returns a printer whether
+  a custom printer for 'val' has been registered or not.
+  """
+  default_printer = gdb.default_visualizer(val)
+  if default_printer:
+    return default_printer
+  # No custom printer available for val, fall back to StructPrinter.
+  return StructPrinter(val)
+
+
+def get_operation_name_printer(val):
+  """Returns printer for an mlir::OperationName instance."""
+  rep_printer = gdb.default_visualizer(val['representation'])
+  if not rep_printer:
+    return None  # If the PointerUnion couldn't be analyzed, print as raw value.
+  pointer = rep_printer.pointer
+  if pointer.type.code == gdb.TYPE_CODE_PTR and pointer:
+    pointer = pointer.dereference()
+  return get_default_or_struct_printer(pointer)
+
+
+def get_first_member_printer(val):
+  """Returns printer for the first member of val."""
+  for field in val.type.fields():
+    if not field.is_base_class:
+      field = val[field.name]
+      if field.type.code == gdb.TYPE_CODE_PTR and field:
+        field = field.dereference()
+      return get_default_or_struct_printer(field)
+  return None
+
+
+def get_first_base_printer(val):
+  """Returns printer for the first base class of val."""
+  for field in val.type.fields():
+    if field.is_base_class and field.type:
+      return get_default_or_struct_printer(val.cast(field.type))
+  return None
+
+
 pp = gdb.printing.RegexpCollectionPrettyPrinter('MLIRSupport')
 
 pp.add_printer('mlir::Identifier', '^mlir::Identifier$', IdentifierPrinter)
+pp.add_printer('mlir::OperationName', '^mlir::OperationName$',
+               get_operation_name_printer)
+
+# Printers forwarding to single relevant member.
+for name in ['Location', 'Value']:
+  pp.add_printer('mlir::%s' % name, '^mlir::%s$' % name,
+                 get_first_member_printer)
+
+# Printers forwarding to the single relevant base class.
+for name in [
+    'BoolAttr', 'LocationAttr', 'ElementsAttr', 'BaseMemRefType', 'FloatType',
+    'ShapedType', 'TensorType'
+]:
+  pp.add_printer('mlir::%s' % name, '^mlir::%s$' % name, get_first_base_printer)
 
 gdb.printing.register_pretty_printer(gdb.current_objfile(), pp)