This is an archive of the discontinued LLVM Phabricator instance.

[clang-repl] Implement pretty printing of custom types.
Needs ReviewPublic

Authored by v.g.vassilev on Mar 24 2023, 6:35 AM.

Details

Summary

This patch is the third part of the below RFC:
https://discourse.llvm.org/t/rfc-handle-execution-results-in-clang-repl/68493

  • It implements Value::dump to enable pretty printing. For example:

clang-repl> int x = 42;
clang-repl> x
(int) 42

It also works for STL facilities:

clang-repl> #include <vector>
clang-repl> std::vector<int> v{1,2,3};
clang-repl> v
(std::vector<int> &) { 1, 2, 3 }

  • Users can also provide their own pretty printers by writing an

overload of PrintValueRuntime(T*). For example:

clang-repl> struct S{};
clang-repl> std::string PrintValueRuntime(S* s) { return "My printer!"; }
clang-repl> S{}
clang-repl> (S) My printer!

Signed-off-by: Jun Zhang <jun@junz.org>

Diff Detail

Event Timeline

junaire created this revision.Mar 24 2023, 6:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2023, 6:35 AM
Herald added a subscriber: mstorsjo. · View Herald Transcript
junaire requested review of this revision.Mar 24 2023, 6:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2023, 6:35 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
junaire updated this revision to Diff 509306.Mar 29 2023, 4:35 AM

Update + Rebase

junaire updated this revision to Diff 510288.Apr 1 2023, 7:42 PM

CHANGELOG:

  1. Add constant array tests.
  2. Fix misc issues.
junaire edited the summary of this revision. (Show Details)Apr 1 2023, 7:44 PM
junaire retitled this revision from [WIP][clang-repl] Implement Value pretty printing to [clang-repl] Implement Value pretty printing.Apr 28 2023, 1:43 AM
junaire edited the summary of this revision. (Show Details)
junaire added a reviewer: v.g.vassilev.
junaire added inline comments.Apr 30 2023, 3:04 AM
clang/test/Interpreter/pretty-print.cpp
108

Hi @mizvekov, do you have a clue about why it doesn't print the correct type? So in ValuePrinter.cpp:50 I dumped the type it shows:

AutoType 0x62d000533c00 'typename _MakeUniq<int>::__single_object' sugar
`-ElaboratedType 0x62d000450500 'typename _MakeUniq<int>::__single_object' sugar
  `-TypedefType 0x62d0004501e0 'std::_MakeUniq<int>::__single_object' sugar
    |-Typedef 0x62d000450160 '__single_object'
    `-ElaboratedType 0x62d000450100 'unique_ptr<int>' sugar
      `-TemplateSpecializationType 0x62d0004500b0 'unique_ptr<int>' sugar unique_ptr
        |-TemplateArgument type 'int':'int'
        | `-SubstTemplateTypeParmType 0x62d00044fd50 'int' sugar typename depth 0 index 0 _Tp
        |   |-ClassTemplateSpecialization 0x62d00044fa08 '_MakeUniq'
        |   `-BuiltinType 0x621000019220 'int'
        `-RecordType 0x62d000450080 'class std::unique_ptr<int>'
          `-ClassTemplateSpecialization 0x62d00044ff70 'unique_ptr'

But in Cling it only has the RecordType. Do you think it's affected by your type resugaring patches?

junaire updated this revision to Diff 518458.May 1 2023, 8:42 AM

Fix AutoType sugar

clang/test/Interpreter/pretty-print.cpp
108

Looks like below diff fixes this:

diff --git a/clang/lib/Interpreter/InterpreterUtils.cpp b/clang/lib/Interpreter/InterpreterUtils.cpp
index 05e6be0e9df2..da5da55e8e95 100644
--- a/clang/lib/Interpreter/InterpreterUtils.cpp
+++ b/clang/lib/Interpreter/InterpreterUtils.cpp
@@ -425,7 +425,7 @@ QualType GetFullyQualifiedType(QualType QT, const ASTContext &Ctx) {
   // Strip deduced types.
   if (const auto *AutoTy = dyn_cast<AutoType>(QT.getTypePtr())) {
     if (!AutoTy->getDeducedType().isNull())
-      return GetFullyQualifiedType(AutoTy->getDeducedType(), Ctx);
+      return GetFullyQualifiedType(AutoTy->getDeducedType().getDesugaredType(Ctx), Ctx);
   }
junaire updated this revision to Diff 520913.May 9 2023, 11:07 PM

Remove unused code.

junaire updated this revision to Diff 520914.May 9 2023, 11:10 PM

Add some comments

junaire updated this revision to Diff 520998.May 10 2023, 8:04 AM

Export symbols in Windows.

junaire updated this revision to Diff 521961.May 13 2023, 9:06 PM

Don't use C++17 because Clang on Windows is not default to that :(

junaire updated this revision to Diff 521962.May 13 2023, 9:21 PM

Add macro guard.

junaire updated this revision to Diff 521981.May 14 2023, 1:20 AM
  • include <tuple> to avoid incorrect lookup on Windows
  • Add -Xcc -fno-delayed-template-parsing to fix failure on Windows
junaire updated this revision to Diff 521985.May 14 2023, 3:38 AM

Make and use our own std::void_t

Invite more people to the party :)

junaire updated this revision to Diff 524093.May 21 2023, 4:22 AM

add caas namespace

I believe this is heading in a good direction. Here are some comments from a partial review. Also perhaps we should extend the patch description (future commit message) to capture the idea of the pretty-printing including the dispatch, the approach how we build the AST and how to write a custom printer.

clang/lib/Interpreter/Value.cpp
264

We should add some documentation for users how to write a pretty-printer for a custom class.

We do not seem to support the multiple inheritance case where one of the base classes has a pretty-printer and the other does not. See the explanation in Cling. It is okay to not have it at the moment but we should include a FIXME here saying that we do not yet support it.

clang/lib/Interpreter/ValuePrinter.cpp
202

Similarly to the way we rebuild the AST during template instantiation, when we are synthesizing AST we do not need the lexical scope information generally.

Removing this need for the parser here will allow us to remove Interpreter::getParser which we should not expose as an API at that stage.

302

I'd prefer to inline to body of CompileDecl here instead of exposing it in the interpreter class.

clang/tools/clang-repl/CMakeLists.txt
40 ↗(On Diff #524096)

We should check if there is a better way to export a subset of symbols through the llvm build system. Maybe @lhames, @sunho or @sgraenitz know how.

I didn't have the chance to complete my review (this is a pretty massive change), but here are some in-progress comments.

clang/include/clang/Interpreter/Interpreter.h
100

const mismatch here -- should be an overload set where the const member function returns a const reference and the non-const member function returns the non-const reference. (This is true in general, I see we've got other such functions that are const-qualified but returning non-const references.)

(Not critical to fix, but would be a nice follow-up NFC to correct these sorts of things; retro-fitting const correctness is hard, so we should strive for const correct by default for new code whenever possible.)

clang/lib/Headers/__clang_interpreter_runtime_printvalue.h
2–4

er, not certain the best way to repair this, but wrapping the comment isn't it. Maybe drop the "Incremental Compilation and Execution" bit?

29
34

You should use reserved identifiers where possible so as not to conflict with user-defined macros.

51

I wonder if it makes some degree of sense to use macros to declare these, so that it's easier to see there's a consistent pattern and which types are supported. e.g.,

#define __DECL_PRINT_VALUE_RUNTIME(type) __REPL_EXTERNAL_VISIBILITY std::string PrintValueRuntime(const type *__Ptr)

__DECL_PRINT_VALUE_RUNTIME(void);
__DECL_PRINT_VALUE_RUNTIME(void *);
__DECL_PRINT_VALUE_RUNTIME(bool);
...

#undef __DECL_PRINT_VALUE_RUNTIME

I also wonder: should this header have a C interface so it can be used from a C context, or is the repl context in which this is included only ever C++?

93
97

(and so forth in this header -- basically, every identifier should be in the reserved namespace unless it's the public API being exposed.)

clang/lib/Interpreter/Interpreter.cpp
435

Any way to make this take a const Decl * instead?

clang/lib/Interpreter/InterpreterUtils.cpp
108
111
117–122
133–136
139–140
157–158
163–168

I'll stop commenting on this sort of thing; you should apply this sort of cleanup to the whole patch.

186–189

So the first case is for handling enumerations and otherwise we expect a structure or a union? Similar question below as well.

239
junaire updated this revision to Diff 525420.May 24 2023, 8:22 PM
junaire marked 15 inline comments as done.

Address comments.

junaire added inline comments.May 24 2023, 8:24 PM
clang/lib/Interpreter/Interpreter.cpp
435

DeclGroupRef in line 439 asks a non-const Decl*, should we use const Decl* and const cast it?

clang/lib/Interpreter/Value.cpp
264

Where should I put the doc? Value.cpp is the implementation and it's unlikely for regular users to read it.

junaire marked an inline comment as done.May 24 2023, 8:35 PM
junaire updated this revision to Diff 525425.May 24 2023, 8:35 PM

Remove Interpereter::getParser + More clean up

v.g.vassilev added inline comments.May 28 2023, 5:07 AM
clang/include/clang/Interpreter/Interpreter.h
77

We should move this routine to its user in maybe ValuePrinter.cpp.

clang/tools/clang-repl/CMakeLists.txt
40 ↗(On Diff #524096)

I wonder if that's related to https://reviews.llvm.org/D151620 in some way? That is, does D151620 fix our exports and this section becomes redundant?

junaire updated this revision to Diff 526383.May 28 2023, 11:00 PM

Make CreateUniqName a static helper

junaire added inline comments.May 28 2023, 11:02 PM
clang/tools/clang-repl/CMakeLists.txt
40 ↗(On Diff #524096)

I haven't tested that but I don't think so. The patch you posted is about fixing issues on particular platforms like MinGW.

junaire marked an inline comment as done.May 28 2023, 11:02 PM
junaire edited the summary of this revision. (Show Details)May 28 2023, 11:10 PM
v.g.vassilev added inline comments.May 28 2023, 11:34 PM
clang/lib/Interpreter/IncrementalParser.h
79

Is that used?

clang/lib/Interpreter/ValuePrinter.cpp
263

Let's add a FIXME here. The problem CompileDecl and GenModule intends to solve is that when we synthesize AST we need to inform the rest of the Clang infrastructure about it and attach the produced llvm::Module to the JIT so that it can resolve symbols from it.

In cling that is solved with a RAII object which marks a scope where the synthesis happens and takes care to inform the rest of the infrastructure. We need something similar and a little more robust maybe. Something like ASTSynthesisRAII which ultimately hides this machinery especially from the public API.

443

All of the PrintValueRuntime seem to be duplicating code. We could use the preprocessor to expand these to but it would require some changes which I believe could be done as a separate patch:

  • Perhaps we should be compatible with cling here in terms of naming as that's a public API - there I believe we use printValue.
  • We should try to leverage TemplateBase.cpp::printIntegral for integral types.
  • We should not return std::string here but a const char* and we should provide somewhere storage for them. That would probably make porting to embedded systems easier since on many the <string> header is hard to support.

I believe it would be enough to have a fixme for now.

474

We could use the preprocessor the way we do in Value.cpp to expand this dispatcher.

v.g.vassilev added inline comments.May 28 2023, 11:38 PM
clang/lib/Interpreter/ValuePrinter.cpp
2

Please add the llvm preamble.

junaire updated this revision to Diff 526566.May 30 2023, 3:19 AM
junaire marked 2 inline comments as done.

Add LLVM preamble

junaire added inline comments.May 30 2023, 3:48 AM
clang/lib/Interpreter/ValuePrinter.cpp
263

I can't really get the point. I agree we shouldn't expose Interpreter::GenModule as a public interface but I don't understand the solution.

443
  • I'd propose using PrintValueRuntime here. This is because we have used it across RFC and other documentation so I don't want to change it :(

We should not return std::string here but a const char* and we should provide somewhere storage for them. That would probably make porting to embedded systems easier since on many the <string> header is hard to support.

  • That really touches the dark corner of our design. On the one hand, we try to keep Value itself as lightweight as possible, on the other hand, we use heavy headers like <string> and <type_traits> in value pretty printing. While this is awkward, we can hardly avoid it because the implementation needs to see the definition of these types to know how to print it. So how can we do this on resources limited systems? My point is that perhaps we can keep it as is, because it's very hard to measure how expensive the feature is on the targeting platform. If the system can afford this, then we don't need to worry about anything, but if the system can't even use STL itself, presumably it shouldn't use value printing at all! Note that this doesn't conflicts with the lightness of the value runtime as it SHOULD add as little overhead as possible no matter whether the host can afford it or not.
474

We actually can't because it doesn't strictly match everything. For example Char_S and SChar both map to V.getSChar() IIUC?

aaron.ballman added inline comments.Jun 13 2023, 8:27 AM
clang/lib/Headers/__clang_interpreter_runtime_printvalue.h
2–4

Comment can be re-flowed so this no longer wraps.

45

Need to use a reserved identifier for everything so you don't conflict with user-defined macros.

51

Still wondering about the C interface question.

80

Still need to make sure you're using reserved identifiers for these (should make a pass through the file and get all of the identifiers).

82
clang/lib/Interpreter/Value.cpp
264

I'd expect we'd be updating clang/docs/ClangRepl.rst for this sort of thing.

clang/lib/Interpreter/ValuePrinter.cpp
90–91

This doesn't distinguish between lvalue and rvalue references, which seems like a mistake.

165

Not just encodings, but also: how do you handle unprintable characters (like control characters)?

544

This seems rather reachable, no?

v.g.vassilev commandeered this revision.Jul 12 2023, 3:05 AM
v.g.vassilev edited reviewers, added: junaire; removed: v.g.vassilev.

Jun has moved on in his career and I'd like to take responsibility for this patch.

v.g.vassilev marked 5 inline comments as done.
v.g.vassilev retitled this revision from [clang-repl] Implement Value pretty printing to [clang-repl] Implement pretty printing of custom types..
  • Address several comments
  • Make __clang_interpreter_runtime_printvalue.h C++11 compliant.
  • Support std::vector<bool>
  • Add more tests
v.g.vassilev added inline comments.Jul 17 2023, 8:27 AM
clang/lib/Headers/__clang_interpreter_runtime_printvalue.h
80

Could you check now, I think I fixed all I thought needed fixing.

clang/lib/Interpreter/ValuePrinter.cpp
165

Extended the FIXME.

544

Do you mean that's not exhaustive list that we list here?

Add forgotten file.