Add `Dump` functions to Check, Parse, and Lex #4669

jonmeow · 2024-12-11T20:22:15Z

Provide Check::Dump(context, arg) and similar.
- gdb and lldb should do contextual lookup, and call Dump(*this, Lex::TokenIndex::Invalid) has been tested with gdb.
- Since this is only for debug, keeps the functions fully separated from code.
Uses alwayslink to ensure objects are correctly linked, even though there are no calls.
- -Wno-missing-prototypes is needed when we don't have forward declarations.
Code is not linked in opt builds, using #ifndef NDEBUG.
- This probably could be doing something in BUILD files with a select(), but the #ifndef seemed easier.

This is based on #4620, but uses free functions instead of member functions.

Co-authored-by: Dana Jansens [email protected]

…ffer These methods are useful for calling in a debugger, and will dump verbose information from an id value. Currently this is done in a pretty simple manner. The Lex and Parse methods are each replicated onto the Check::Context class manually (and same for Lex onto Parse::Tree). The DumpId methods are implemented in this PR for: - Lex::TokenIndex - Parse::NodeId - SemIR::LocId

danakj · 2024-12-12T15:14:49Z

This is based on #4620. Versus that, this is doing a few things differently:

Cool thanks for figuring out the free functions approach with bazel. If this merges would it just take the commit description and author from the first commit or will it apply co-authored correctly? We can reword the first commit to describe what the PR ended up doing?

danakj · 2024-12-12T15:04:08Z

toolchain/check/BUILD

@@ -93,6 +91,24 @@ cc_library(
        "//toolchain/sem_ir:typed_insts",
        "@llvm-project//llvm:Support",
    ],
+    alwayslink = 1,


Leave a comment why this is here?

Looks like this one was missed, I also wondered if it's actually needed since dump was moved to another cc_library.

Oops, I wasn't looking closely enough because I just assumed I'd already removed this set. Removed.

danakj · 2024-12-12T15:04:13Z

toolchain/check/BUILD

+        "//toolchain/parse:tree",
+        "//toolchain/sem_ir:file",
+    ],
+    alwayslink = 1,


Leave a comment why this is here?

danakj · 2024-12-12T15:04:20Z

toolchain/check/BUILD

+cc_library(
+    name = "dump_id",
+    srcs = ["dump_id.cpp"],
+    copts = ["-Wno-missing-prototypes"],


Leave a comment why this is here?

danakj · 2024-12-12T15:05:28Z

toolchain/lex/BUILD

+        ":tokenized_buffer",
+        "//common:ostream",
+    ],
+    alwayslink = 1,


Leave a comment why this is here?

danakj · 2024-12-12T15:07:08Z

toolchain/parse/BUILD

+        "//common:ostream",
+        "//toolchain/lex:dump_id",
+    ],
+    alwayslink = 1,


Comment why this is here?

danakj · 2024-12-12T15:08:55Z

toolchain/parse/dump_id.cpp

@@ -26,4 +27,19 @@ auto DumpIdImpl(const Tree& tree, NodeId node_id) -> void {
  llvm::errs() << ")";
 }

+// A set of DumpId() overloads that dump an object to stderr, useful for


In Lex, this is moved to the header, should this comment move to the header here too? Or can we make it consistent either way

Added a more complete comment to each header.

Also a mention in docs/

danakj · 2024-12-12T15:11:42Z

toolchain/check/dump_id.cpp

+
+// A set of DumpId() overloads that dump an object to stderr, useful for
+// calling inside a debugger.
+LLVM_DUMP_METHOD auto DumpId(const Context& context, Lex::TokenIndex token)


The PR description says "this provides Check::Dump(context, arg) and similar" which is a typo (no Id) that we keep making. And we'll want to dump things other than Ids (like PendingBlock). So maybe we should drop the Id suffix. This is also fine for now, but I'm not happy with the naming forever yet. I was also talking with Geoff about maybe renaming "Dump" in Printable (Dump->Print and Print->PrintTo?) or something to avoid the name collisions.

LLVM typically calls it dump, which is where the name Dump came from:

https://github.com/search?q=repo%3Allvm%2Fllvm-project%20LLVM_DUMP_METHOD&type=code

(ed: to be clear, I think there's significant value in consistency)

You're correct the PR description doesn't match, I'll change the call though.

Note, removing Dump from Printable would seem fine to me (I only added it for debugging, for others), I'm just meaning that if there's going to be a function that does dumping, it should probably use the familiar name.

Right yeah I don't want to diverge from LLVM too much either. My full opinion state here is that I'd prefer Dump to be used for the place that includes as much info as possible. The dump() in LLVM does include comprehensive amount of info generally everything you need when debugging, similarly. I'd personally be fine with renaming or removing the Dump function from Printable. I haven't had a lot of use of calling Dump explicitly on Printable types, as they are already streamable.

(to be sure, I'm assuming this'll be resolved separately)

yes, me too

jonmeow · 2024-12-12T18:32:24Z

This is based on #4620. Versus that, this is doing a few things differently:

Cool thanks for figuring out the free functions approach with bazel. If this merges would it just take the commit description and author from the first commit or will it apply co-authored correctly? We can reword the first commit to describe what the PR ended up doing?

Sure, and since it sounds like you're okay proceeding with this approach, would it make sense to close #4620?

AFAIU GitHub adds Co-authored-by lines automatically. You can find examples in PRs such as #4623 -- note that I have commits in that branch, and GitHub attributes me in daba2c7

danakj · 2024-12-12T20:14:12Z

toolchain/check/BUILD

@@ -93,6 +91,24 @@ cc_library(
        "//toolchain/sem_ir:typed_insts",
        "@llvm-project//llvm:Support",
    ],
+    alwayslink = 1,


Looks like this one was missed, I also wondered if it's actually needed since dump was moved to another cc_library.

danakj · 2024-12-12T20:15:03Z

toolchain/check/context.h

@@ -6,6 +6,7 @@
 #define CARBON_TOOLCHAIN_CHECK_CONTEXT_H_

 #include "common/map.h"
+#include "common/ostream.h"


This is no longer used in this PR and can be removed.

danakj · 2024-12-12T20:15:12Z

toolchain/check/BUILD

@@ -72,6 +72,7 @@ cc_library(
        "//common:array_stack",
        "//common:check",
        "//common:map",
+        "//common:ostream",


This is no longer used in this PR and can be removed.

- Provide `Check::Dump(context, arg)` and similar. - gdb and lldb should do contextual lookup, and `call Dump(*this, Lex::TokenIndex::Invalid)` has been tested with gdb. - Since this is only for debug, keeps the functions fully separated from code. - Uses alwayslink to ensure objects are correctly linked, even though there are no calls. - `-Wno-missing-prototypes` is needed when we don't have forward declarations. - Code is not linked in opt builds, using `#ifndef NDEBUG`. - This probably could be doing something in BUILD files with a `select()`, but the `#ifndef` seemed easier. This is based on carbon-language#4620, but uses free functions instead of member functions. Co-authored-by: Dana Jansens <[email protected]> --------- Co-authored-by: danakj <[email protected]>

Follow-on to #4669 . Did some manual testing, but may have not exercised all code paths. --------- Co-authored-by: Josh L <[email protected]>

danakj and others added 18 commits December 5, 2024 14:35

add-missing-files

9cc9ac4

parse-tree

e28d533

iwyu

30bb063

dump import locations

19fddbc

dump-methods-in-base-classes

08907b1

add-parse-and-overload-of-lex

1526f1d

use-if-constexpr

bdc9c01

simplify

b0b1cdf

simply

91bc8a0

add-dump-content

506a504

review-feedback

fb7840f

write-newline

228273f

Merge branch 'trunk' into dump

a8da68b

dont-avoid-include-ostream

3ed5149

remove-base-class

2bec55b

Merge remote-tracking branch 'refs/remotes/origin/trunk' into dump

d5a2374

Switch to free functions

1964585

jonmeow requested review from danakj and geoffromer December 11, 2024 20:22

github-actions bot added the toolchain label Dec 11, 2024

github-actions bot requested a review from josh11b December 11, 2024 20:22

jonmeow mentioned this pull request Dec 11, 2024

Add DumpId() methods to Check::Context, Parse::Tree, Lex::TokenizedBuffer #4620

Closed

jonmeow removed the request for review from josh11b December 11, 2024 20:24

danakj reviewed Dec 12, 2024

View reviewed changes

jonmeow added 3 commits December 12, 2024 11:27

Review cleanup

4fa1b64

prototypes

c9d28da

doc

3365c6c

danakj approved these changes Dec 12, 2024

View reviewed changes

danakj changed the title ~~Add DumpId functions to Check, Parse, and Lex~~ Add Dump functions to Check, Parse, and Lex Dec 12, 2024

Address comments

baa3dda

jonmeow enabled auto-merge December 12, 2024 20:39

jonmeow added this pull request to the merge queue Dec 12, 2024

Merged via the queue into carbon-language:trunk with commit 3ce0df6 Dec 12, 2024
8 checks passed

jonmeow deleted the dump-free-fn branch December 12, 2024 21:17

josh11b mentioned this pull request Dec 27, 2024

Add more Dump methods for debugging #4747

Merged

github-merge-queue bot pushed a commit that referenced this pull request Jan 6, 2025

Add more Dump methods for debugging (#4747)

7edb0b8

Follow-on to #4669 . Did some manual testing, but may have not exercised all code paths. --------- Co-authored-by: Josh L <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Dump` functions to Check, Parse, and Lex #4669

Add `Dump` functions to Check, Parse, and Lex #4669

jonmeow commented Dec 11, 2024 •

edited

Loading

danakj commented Dec 12, 2024

danakj Dec 12, 2024

jonmeow Dec 12, 2024

danakj Dec 12, 2024

jonmeow Dec 12, 2024

danakj Dec 12, 2024

jonmeow Dec 12, 2024

danakj Dec 12, 2024

jonmeow Dec 12, 2024

danakj Dec 12, 2024

jonmeow Dec 12, 2024

danakj Dec 12, 2024

jonmeow Dec 12, 2024

danakj Dec 12, 2024

jonmeow Dec 12, 2024

jonmeow Dec 12, 2024

danakj Dec 12, 2024

jonmeow Dec 12, 2024 •

edited

Loading

jonmeow Dec 12, 2024 •

edited

Loading

danakj Dec 12, 2024

jonmeow Dec 12, 2024

danakj Dec 12, 2024

jonmeow commented Dec 12, 2024

danakj Dec 12, 2024

danakj Dec 12, 2024

jonmeow Dec 12, 2024

danakj Dec 12, 2024

jonmeow Dec 12, 2024

Add Dump functions to Check, Parse, and Lex #4669

Add Dump functions to Check, Parse, and Lex #4669

Conversation

jonmeow commented Dec 11, 2024 • edited Loading

danakj commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

jonmeow Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonmeow commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `Dump` functions to Check, Parse, and Lex #4669

Add `Dump` functions to Check, Parse, and Lex #4669

jonmeow commented Dec 11, 2024 •

edited

Loading

jonmeow Dec 12, 2024 •

edited

Loading

jonmeow Dec 12, 2024 •

edited

Loading