Page MenuHomePhabricator

Add GDB prettyprinters for a few more MLIR types.
ClosedPublic

Authored by csigg on Sep 4 2020, 1:06 PM.

Diff Detail

Event Timeline

csigg created this revision.Sep 4 2020, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2020, 1:06 PM
csigg requested review of this revision.Sep 4 2020, 1:07 PM
csigg updated this revision to Diff 290015.Sep 4 2020, 1:08 PM

Removing unnecessary #includes.

csigg updated this revision to Diff 290018.Sep 4 2020, 1:20 PM

Add missing newline at end of file.

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!

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 updated this revision to Diff 293153.Sep 21 2020, 6:23 AM

Clarify documentation.

csigg added inline comments.Sep 21 2020, 7:00 AM
mlir/utils/gdb-scripts/prettyprinters.py
37–44

I hope this is clearer now.

dblaikie added inline comments.Sep 22 2020, 6:59 PM
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 added inline comments.Sep 23 2020, 2:22 AM
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 added inline comments.Sep 23 2020, 6:52 PM
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 updated this revision to Diff 294703.Sep 28 2020, 7:44 AM

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

csigg added inline comments.Sep 28 2020, 7:45 AM
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 updated this revision to Diff 294796.Sep 28 2020, 1:22 PM

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 accepted this revision.Sep 28 2020, 1:26 PM

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.

This revision is now accepted and ready to land.Sep 28 2020, 1:26 PM

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 updated this revision to Diff 294952.Sep 29 2020, 6:22 AM

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

Fix llvm-support test to work with RelWithDebInfo.

jpienaar accepted this revision.Sep 29 2020, 7:46 AM

Thanks for doing this!

debuginfo-tests/llvm-prettyprinters/gdb/llvm-support.gdb
1 ↗(On Diff #294952)

Left over from debugging?

csigg added inline comments.Sep 29 2020, 11:12 AM
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 added inline comments.Sep 29 2020, 11:20 AM
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 updated this revision to Diff 295225.Sep 30 2020, 3:09 AM

Remove --dump-input=fail.

csigg added inline comments.Sep 30 2020, 3:11 AM
debuginfo-tests/llvm-prettyprinters/gdb/llvm-support.gdb
1 ↗(On Diff #294952)

Indeed, this was cargo cult.

This revision was landed with ongoing or failed builds.Sep 30 2020, 12:23 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added a subscriber: MaskRay.EditedJan 6 2021, 9:03 PM

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.

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?

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.