Page MenuHomePhabricator

[NFC] Refactor DiagnosticBuilder and PartialDiagnostic
ClosedPublic

Authored by yaxunl on Jul 22 2020, 2:07 PM.

Details

Summary

PartialDiagnostic misses some functions compared to DiagnosticBuilder.

This patch refactors DiagnosticBuilder and PartialDiagnostic, extracts
the common functionality so that the streaming << operators are
shared.

Diff Detail

Event Timeline

yaxunl created this revision.Jul 22 2020, 2:07 PM
tra added a comment.Jul 22 2020, 2:37 PM

FYI, the patch does not compile:

In file included from llvm/repo/clang/lib/Parse/ParseStmtAsm.cpp:13:
In file included from /llvm/repo/clang/include/clang/Parse/Parser.h:24:
llvm/repo/clang/include/clang/Sema/Sema.h:1833:10: error: use of overloaded operator '<<' is ambiguous (with operand types 'const clang::Sema::SemaDiagnosticBuilder' and 'clang::QualType')
      DB << T;
      ~~ ^  ~
llvm/repo/clang/include/clang/AST/Type.h:7144:28: note: candidate function [with DiagBuilderT = clang::Sema::SemaDiagnosticBuilder, $1 = nullptr]
inline const DiagBuilderT &operator<<(const DiagBuilderT &DB, QualType T) {
                           ^
llvm/repo/clang/include/clang/Sema/Sema.h:1508:41: note: candidate function [with T = clang::QualType]
    friend const SemaDiagnosticBuilder &operator<<(
                                        ^
llvm/repo/clang/include/clang/AST/TemplateBase.h:684:26: note: candidate function
const DiagnosticBuilder &operator<<(const DiagnosticBuilder &DB,
yaxunl updated this revision to Diff 279951.Jul 22 2020, 2:58 PM

fix build failure

So, the idea here is to do some sort of duck-typing and allow DiagBuilder to work with both DiagnosticBuilder and PartialDiagnostic.

What bothers me is that unlike Diagnostic PartialDiagnostic seems to be commingling functionality of the builder with that of the diagnostic itself. I'm not sure if that's by design or just happened to be that way.

I think a better approach would be to refactor PartialDiagnostic and separate the builder functionality. That should make it possible to create a common diagnostic builder base class with Partial/Full diagnostic deriving their own builder, if needed.

That said, I'm not that familiar with the diags. Perhaps @rtrieu @aaron.ballman would have better ideas.

aaron.ballman added a subscriber: rsmith.
In D84362#2271585, @tra wrote:

So, the idea here is to do some sort of duck-typing and allow DiagBuilder to work with both DiagnosticBuilder and PartialDiagnostic.

What bothers me is that unlike Diagnostic PartialDiagnostic seems to be commingling functionality of the builder with that of the diagnostic itself. I'm not sure if that's by design or just happened to be that way.

I think a better approach would be to refactor PartialDiagnostic and separate the builder functionality. That should make it possible to create a common diagnostic builder base class with Partial/Full diagnostic deriving their own builder, if needed.

That said, I'm not that familiar with the diags. Perhaps @rtrieu @aaron.ballman would have better ideas.

I'm similarly a bit uncomfortable with adding the SFINAE magic to make this work instead of making a base class that will work for either kind of diagnostic builder. I'm adding @rsmith to see if he has opinions as well.

In D84362#2271585, @tra wrote:

So, the idea here is to do some sort of duck-typing and allow DiagBuilder to work with both DiagnosticBuilder and PartialDiagnostic.

What bothers me is that unlike Diagnostic PartialDiagnostic seems to be commingling functionality of the builder with that of the diagnostic itself. I'm not sure if that's by design or just happened to be that way.

I think a better approach would be to refactor PartialDiagnostic and separate the builder functionality. That should make it possible to create a common diagnostic builder base class with Partial/Full diagnostic deriving their own builder, if needed.

That said, I'm not that familiar with the diags. Perhaps @rtrieu @aaron.ballman would have better ideas.

I'm similarly a bit uncomfortable with adding the SFINAE magic to make this work instead of making a base class that will work for either kind of diagnostic builder. I'm adding @rsmith to see if he has opinions as well.

There are use patterns expecting PartialDiagnosticInst << X << Y to continue to be a PartialDiagnostic&, e.g.

PartialDiagnosticAt PDAt(SourceLoc, PartialDiagnosticInst << X << Y);

However if we derive PartialDiagnostic and DiagnosticBuilder from a base class DiagnosticBuilderBase which implements the << operators, PartialDiagnosticInst << X << Y will become a DiagnosticBuilderBase&, then we can no longer write the above code.

That's one reason I use templates to implement << operators.

Do we want to sacrifice this convenience?

tra added a comment.Sep 15 2020, 11:32 AM

There are use patterns expecting PartialDiagnosticInst << X << Y to continue to be a PartialDiagnostic&, e.g.

PartialDiagnosticAt PDAt(SourceLoc, PartialDiagnosticInst << X << Y);

However if we derive PartialDiagnostic and DiagnosticBuilder from a base class DiagnosticBuilderBase which implements the << operators, PartialDiagnosticInst << X << Y will become a DiagnosticBuilderBase&, then we can no longer write the above code.

That's one reason I use templates to implement << operators.

Do we want to sacrifice this convenience?

I don't think we have to.
AFAICT, virtually all such patterns (and there are only 20-ish of them in total) are used with EmitFormatDiagnostic(S.PDiag()) which could be adapted to accept DiagnosticBuilderBase and Sema::PDiag() changed to return PartialDiagnosticBuilder with no loss of convenience.

In D84362#2274884, @tra wrote:

There are use patterns expecting PartialDiagnosticInst << X << Y to continue to be a PartialDiagnostic&, e.g.

PartialDiagnosticAt PDAt(SourceLoc, PartialDiagnosticInst << X << Y);

However if we derive PartialDiagnostic and DiagnosticBuilder from a base class DiagnosticBuilderBase which implements the << operators, PartialDiagnosticInst << X << Y will become a DiagnosticBuilderBase&, then we can no longer write the above code.

That's one reason I use templates to implement << operators.

Do we want to sacrifice this convenience?

I don't think we have to.
AFAICT, virtually all such patterns (and there are only 20-ish of them in total) are used with EmitFormatDiagnostic(S.PDiag()) which could be adapted to accept DiagnosticBuilderBase and Sema::PDiag() changed to return PartialDiagnosticBuilder with no loss of convenience.

I saw lots of usage of PartialDiagnosticAt, which is defined at

https://github.com/llvm/llvm-project/blob/8c3d0d6a5f5a2a521c4cbae7acbad82a49e2a92f/clang/include/clang/Basic/PartialDiagnostic.h#L417

It requres a PartialDiagnostic as the second argument.

A typical use is like

https://github.com/llvm/llvm-project/blob/69f98311ca42127df92527b6fc3be99841a15f12/clang/lib/Sema/AnalysisBasedWarnings.cpp#L1741

If we use a base class DiagnosticBuilderBase to implement << operators, S.PDiag(diag::warn_unlock_but_no_lock) << Kind << LockName becomes DiagnosticBuilderBase& and we can no longer write the above code.

There are ~100 uses like that.

tra added a comment.Sep 15 2020, 1:46 PM

Perhaps we could inherit PartialDiagnostic from DiagnosticBuilder base class. This would probably be the least invasive approach as it would preserve existing uses while allowing reuse of common DiagnosticBuilder functionality. That said, it would muddle the meaning of PartialDiagnostic even more -- is it the diagnostic itself, or a thing that produces the diagnostic?

To think of it, we actually don't need the DiagnosticBuilderBase per se, but rather a limited subset of operations for implementing freestanding operator<<. So the base class for DiagnosticBuilder and PartialDiagnostic could be called StreamableDiagnosticBase and only provide those bits. This way the arrangement looks a bit less awkward. Both DiagnosticBuilder and PartialDiagnostic would inherit from it and would work with operator<<(StreamableDiagnosticBase). We may need to convert StreamableDiagnosticBase results back to the original type.

yaxunl updated this revision to Diff 292319.Sep 16 2020, 1:10 PM
yaxunl retitled this revision from [NFC] Add missing functions to PartialDiagnostic to [NFC] Refactor DiagnosticBuilder and PartialDiagnostic.
yaxunl edited the summary of this revision. (Show Details)

Revised by Artem's comments. Extracted the common code of DiagnosticBuilder and PartialDiagnostic
and removed redundant code.

tra accepted this revision.Sep 16 2020, 1:25 PM

This is a very nice cleanup. Thank you, Sam.

This revision is now accepted and ready to land.Sep 16 2020, 1:25 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 16 2020, 2:36 PM
tra added a comment.Sep 17 2020, 10:13 AM

Apparently this patch triggers compiler crashes on some of our code. I'll try to create a reproducer, but it would be great to revert the patch for now.

*** SIGSEGV (@0x7f68892d8fe8),  received by PID 8210 (TID 8225) on cpu 65; stack trace: ***
PC: @     0x7f686d8a169d  (unknown)  clang::Decl::getAvailability()
    @     0x7f6712fad13b       1376  FailureSignalHandler()
    @     0x7f696f1b69a0      34448  (unknown)
    @     0x7f688c278c9a        352  ShouldDiagnoseAvailabilityOfDecl()
    @     0x7f688c27897d        352  clang::Sema::DiagnoseAvailabilityOfDecl()
    @     0x7f688c8bd3c7       1584  clang::Sema::DiagnoseUseOfDecl()
    @     0x7f688c8cc1e8        672  clang::Sema::BuildDeclarationNameExpr()
    @     0x7f688c8c9e30        192  clang::Sema::BuildDeclarationNameExpr()
    @     0x7f688c8c2698       1280  clang::Sema::ActOnIdExpression()
    @     0x7f688e9b95db       9136  clang::Parser::ParseCastExpression()
    @     0x7f688e9b358b        128  clang::Parser::ParseCastExpression()
    @     0x7f688e9b9ebb       9136  clang::Parser::ParseCastExpression()
    @     0x7f688e9b358b        128  clang::Parser::ParseCastExpression()
    @     0x7f688e9b1998        112  clang::Parser::ParseAssignmentExpression()
    @     0x7f688e9b1832         64  clang::Parser::ParseExpression()
    @     0x7f688e9bd6f0       4064  clang::Parser::ParseParenExpression()
    @     0x7f688e9b6bb4       9136  clang::Parser::ParseCastExpression()
    @     0x7f688e9b358b        128  clang::Parser::ParseCastExpression()
    @     0x7f688e9b1998        112  clang::Parser::ParseAssignmentExpression()
    @     0x7f688e9c1285        288  clang::Parser::ParseExpressionList()
    @     0x7f688e9b4731       3088  clang::Parser::ParsePostfixExpressionSuffix()
    @     0x7f688e9bb92a       9136  clang::Parser::ParseCastExpression()
    @     0x7f688e9b358b        128  clang::Parser::ParseCastExpression()
    @     0x7f688e9b1998        112  clang::Parser::ParseAssignmentExpression()
    @     0x7f688e9b1832         64  clang::Parser::ParseExpression()
    @     0x7f688ea43040        240  clang::Parser::ParseReturnStatement()
    @     0x7f688ea3cef0       1264  clang::Parser::ParseStatementOrDeclarationAfterAttributes()
    @     0x7f688ea3c5cf        160  clang::Parser::ParseStatementOrDeclaration()
    @     0x7f688ea4490a       1264  clang::Parser::ParseCompoundStatementBody()
    @     0x7f688ea45df9        272  clang::Parser::ParseFunctionStatementBody()
    @     0x7f688ea6a7b4        960  clang::Parser::ParseFunctionDefinition()
    @     0x7f688e9600bd       2880  clang::Parser::ParseDeclGroup()
    @     0x7f688ea69634        368  clang::Parser::ParseDeclOrFunctionDefInternal()
    @     0x7f688ea68c20        672  clang::Parser::ParseDeclarationOrFunctionDefinition()
    @     0x7f688ea684ee        768  clang::Parser::ParseExternalDeclaration()
    @     0x7f688ea667a9        368  clang::Parser::ParseTopLevelDecl()
    @     0x7f688e93d2a2        240  clang::ParseAST()
    @     0x7f688fe079bd         80  clang::ASTFrontendAction::ExecuteAction()
    @     0x7f688fe073b8        144  clang::FrontendAction::Execute()
    @     0x7f688fd1bdd6        480  clang::ASTUnit::Parse()
tra added a comment.Sep 17 2020, 11:05 AM
In D84362#2279688, @tra wrote:

Apparently this patch triggers compiler crashes on some of our code. I'll try to create a reproducer, but it would be great to revert the patch for now.

It's likely the same issue as the one reported in D84364, but we probably triggered it independently.

In D84362#2279845, @tra wrote:
In D84362#2279688, @tra wrote:

Apparently this patch triggers compiler crashes on some of our code. I'll try to create a reproducer, but it would be great to revert the patch for now.

It's likely the same issue as the one reported in D84364, but we probably triggered it independently.

I have a fix for the issue reported in D84364. Would you like to try? Thanks.

tra reopened this revision.Sep 18 2020, 2:27 PM

I have a fix for the issue reported in D84364. Would you like to try? Thanks.

I can try it on the internal test that crashed with the patch. I've reopened this review and will pick up the diff once you update it.

This revision is now accepted and ready to land.Sep 18 2020, 2:27 PM
In D84362#2282965, @tra wrote:

I have a fix for the issue reported in D84364. Would you like to try? Thanks.

I can try it on the internal test that crashed with the patch. I've reopened this review and will pick up the diff once you update it.

The fix is for the change in D84364. It has no effect on the change in this review. Are you sure the issue you saw is due to change in this review instead of change in D84364?

tra added a comment.Sep 18 2020, 2:52 PM

The fix is for the change in D84364. It has no effect on the change in this review. Are you sure the issue you saw is due to change in this review instead of change in D84364?

Pretty sure. We've bisected the failure specifically to commit ee5519d323571c4a9a7d92cb817023c9b95334cd, before D84364 landed.

In D84362#2283045, @tra wrote:

The fix is for the change in D84364. It has no effect on the change in this review. Are you sure the issue you saw is due to change in this review instead of change in D84364?

Pretty sure. We've bisected the failure specifically to commit ee5519d323571c4a9a7d92cb817023c9b95334cd, before D84364 landed.

I think this is probably a different issue. The issue reported in D84364 was introduced by change in D84364.

tra added a comment.Sep 18 2020, 2:59 PM

I think this is probably a different issue. The issue reported in D84364 was introduced by change in D84364.

It's possible. Unfortunately it's only triggered by our internal tool and it's hard to create a public reproducer for it. I'll debug and try to fix it on Monday.

tra accepted this revision.Sep 21 2020, 12:23 PM
In D84362#2283078, @tra wrote:

It's possible. Unfortunately it's only triggered by our internal tool and it's hard to create a public reproducer for it. I'll debug and try to fix it on Monday.

It appears the patch slightly increased stack use and caused a failure in code that was running with a limited stack space available.
AFAICT there are no issues with this patch per se and it's good to re-land, again.

rnk added inline comments.
clang/include/clang/Basic/Diagnostic.h
1086–1090

This comment documents that this object is intended to be small. Adding a vtable grows it by one pointer, and the virtual methods practically ensure that it will always be address-taken and will not be promoted from memory to registers. I think you need to reconsider the design of this patch. I will revert it for now. Please get approval from @rsmith or @rjmccall before adding a vtable to this class gain.

tra requested changes to this revision.Thu, Sep 24, 11:32 AM
tra added a reviewer: rjmccall.
This revision now requires changes to proceed.Thu, Sep 24, 11:32 AM
rjmccall added inline comments.Thu, Oct 1, 11:44 PM
clang/include/clang/Basic/Diagnostic.h
1086–1090

I don't know that copying a DiagnosticBuilder is actually an important thing to support, but yes, this doesn't really seem like the right approach. The bigger problem is that it still leaves an enormous amount of code duplication between the active diagnostic being built in the DiagnosticsEngine and the diagnostic being built in the PartialDiagnostic — it's basically the same storage concept, duplicated in two different places! We should unify those two things, so that there's only one class implementing the storage of an active diagnostic. There can then be a common object of that class in DiagnosticsEngine, which DiagnosticBuilder then stores a reference to, while PartialDiagnostic has its own copy. And then it should be fairly straightforward to make the operators work with either.

Among other things, that might make it significantly more efficient to emit a diagnostic from a PartialDiagnostic — we might be able to just emit it from the internal storage instead of having to copy it into the engine, so that the engine's use of a common diagnostic object is just a storage optimization.

yaxunl updated this revision to Diff 298125.Wed, Oct 14, 5:17 AM

revised by John's comments.

Extracted common part of DiagnosticEngine and PartialDiagnostics as DiagnosticStorage.

Make member functions of the base class of DiagnosticBuilder and ParticalDiagnostics non-virtual.

tra added inline comments.Wed, Oct 14, 11:08 AM
clang/include/clang/Basic/PartialDiagnostic.h
193

Is there a particular reason to move field initialization into the body here and in other constructors?

yaxunl marked an inline comment as done.Wed, Oct 14, 11:25 AM
yaxunl added inline comments.
clang/include/clang/Basic/PartialDiagnostic.h
193

Allocator is now a member of the base class, therefore needs to be initialized directly. I could add ctor to the base class but I did not see too much benefit of doing that.

Thanks, this looks a lot better.

clang/include/clang/Basic/Diagnostic.h
1055

I think I would prefer StreamingDiagnostic as the class name here.

clang/include/clang/Basic/PartialDiagnostic.h
224

Why are these template operators necessary? The LHS of the << should convert to a base reference type just fine.

yaxunl updated this revision to Diff 298368.Thu, Oct 15, 7:08 AM
yaxunl marked an inline comment as done.

Rename StreamableDiagnosticBase to StreamingDiagnostic.

yaxunl marked 4 inline comments as done.Thu, Oct 15, 7:13 AM
yaxunl added inline comments.
clang/include/clang/Basic/Diagnostic.h
1055

renamed

clang/include/clang/Basic/PartialDiagnostic.h
224

There are lots of usage patterns expecting the derived class after streaming, e.g.

void foo(PartialDiagnostic& PD);
foo(PD << X << Y);
rjmccall added inline comments.Thu, Oct 15, 10:47 AM
clang/include/clang/Basic/PartialDiagnostic.h
224

I see. You could also just do this in the base operators by just making them templates, e.g.

template <class Diagnostic>
std::enable_if_t<std::is_base_of_v<StreamingDiagnostic, Diagnostic>, const Diagnostic &>
operator<<(const Diagnostic &D, ...) {
}

That way you'd never have these forwarding problems — but it would be more boilerplate for each operator, so maybe it's a wash.

yaxunl updated this revision to Diff 299044.Mon, Oct 19, 7:26 AM
yaxunl marked 2 inline comments as done.

Add constructors to StreamingDiagnostic.

rjmccall accepted this revision.Mon, Oct 19, 10:59 AM

Okay. This LGTM if you feel that the operator forwarders is the better approach.

This revision was not accepted when it landed; it landed in state Needs Review.Mon, Oct 19, 2:48 PM
This revision was automatically updated to reflect the committed changes.