Page MenuHomePhabricator

[NFC] Refactor DiagnosticBuilder and PartialDiagnostic
AcceptedPublic

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.Tue, Sep 15, 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.Tue, Sep 15, 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.Wed, Sep 16, 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.Wed, Sep 16, 1:25 PM

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

This revision is now accepted and ready to land.Wed, Sep 16, 1:25 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 16, 2:36 PM
tra added a comment.Thu, Sep 17, 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.Thu, Sep 17, 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.Fri, Sep 18, 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.Fri, Sep 18, 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.Fri, Sep 18, 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.Fri, Sep 18, 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.