This is an archive of the discontinued LLVM Phabricator instance.

[Modules] Change result of reading AST block to llvm::Error instead
ClosedPublic

Authored by bnbarham on Aug 17 2021, 9:25 PM.

Details

Summary

Reading the AST block can never fail with a recoverable error as modules
cannot be removed during this phase. Change the return type of these
functions to return an llvm::Error instead, ie. either success or
failure.

NFC other than the wording of some of the errors.

Diff Detail

Event Timeline

bnbarham requested review of this revision.Aug 17 2021, 9:25 PM
bnbarham created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2021, 9:25 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Not super happy with the complexity DiagnosedError introduces, want to consider alternatives first.

clang/lib/Serialization/ASTReader.cpp
3762

Good copy-paste catch.

bnbarham added a comment.EditedAug 20 2021, 3:20 PM

Not super happy with the complexity DiagnosedError introduces, want to consider alternatives first.

Ah yes, I wasn't happy with that either but couldn't come up with a decent alternative. I also considered using DiagnosticError but we'd need to keep a DiagStorageAllocator around in that case.

Honestly I don't love Error in general. It'd be nice if DiagnosticsEngine handled multiple in-flight diagnostics.

EDIT: I just noticed that ASTContext has a getDiagAllocator, so maybe DiagnosticError is the way to go?

DiagnosticError looks like a good fit for the task at hand, so it is worth to try it. Though I don't know if it would end up in the end convoluted or OK.

LGTM once @vsapsai is happy.

DiagnosticError looks like a good fit for the task at hand, so it is worth to try it. Though I don't know if it would end up in the end convoluted or OK.

Unless we also change DiagnosticEngine it doesn't look like this is a viable solution. The PartialDiagnostic can't be emitted straight to Diags, since there may already be a diagnostic in flight (see Error(unsigned DiagID, ...)). The args in PartialDiagnostic are currently protected and it seems weird to change that, but even if they weren't it sort of defeats the purpose of using DiagnosticError in the first place.

Any other ideas?

Unless we also change DiagnosticEngine it doesn't look like this is a viable solution. The PartialDiagnostic can't be emitted straight to Diags, since there may already be a diagnostic in flight (see Error(unsigned DiagID, ...)). The args in PartialDiagnostic are currently protected and it seems weird to change that, but even if they weren't it sort of defeats the purpose of using DiagnosticError in the first place.

Any other ideas?

This might be a stupid idea and a bridge too far but what if delayed diagnostic was storing PartialDiagnostic and not three strings? This looks like a better API but I haven't tried it myself and concerned we might not have diag allocator in all required places.

Another idea is to replace DiagnosedError with something like ThreeStringError (please don't use this name). I can be wrong but I find it easier to understand an error representing diagnostics compared to a marker that diagnostic was emitted-or-scheduled earlier.

! In D108268#2961735, @vsapsai wrote:
This might be a stupid idea and a bridge too far but what if delayed diagnostic was storing PartialDiagnostic and not three strings? This looks like a better API but I haven't tried it myself and concerned we might not have diag allocator in all required places.

It seems like it should be possible to not have the "only one in-flight diagnostic" restriction at all. Removing that would be nice since 1. we could just use DiagnosticError and 2. there would be no need to check for an existing diagnostic.

@yaxunl I see you did a bunch of work extracting out the DiagnosticStorage and StreamingDiagnostic classes. Any thoughts on this? I feel like I'm missing something obvious here.

Another idea is to replace DiagnosedError with something like ThreeStringError (please don't use this name). I can be wrong but I find it easier to understand an error representing diagnostics compared to a marker that diagnostic was emitted-or-scheduled earlier.

There's only a few places that call SetDelayedDiagnostic, but none of them would have an allocator (ie. DiagnosticIDs and ContentCache). I may fallback to this instead (could just call it ASTReadError/something similar).

bnbarham updated this revision to Diff 368781.Aug 25 2021, 5:18 PM

Ended up just using DiagnosticError and reading the args from PartialDiagnostic. Still not really happy with this, but I think the alternative of allowing multiple in-flight diagnostics should be a separate PR regardless (if done at all).

vsapsai accepted this revision.Aug 25 2021, 7:19 PM

Allowing multiple PartialDiagnostic to be in-flight/delayed is a nice goal but it's not clear if it's worth it. Also I'm not sure it is worth investing into making delayed diagnostic easier to use. But that is a separate change and the current one looks fine.

clang/lib/Serialization/ASTReader.cpp
1284

Don't have a strong opinion but an alternative to preserve more of the old code and avoid manual SmallVector manipulations can be

llvm::Error RemainingErr = handleErrors(std::move(Err), /*DiagnosticError lambda*/);
if (RemainingErr)
  Error(toString(std::move(RemainingErr)));
This revision is now accepted and ready to land.Aug 25 2021, 7:19 PM
bnbarham updated this revision to Diff 368798.Aug 25 2021, 8:22 PM
bnbarham marked 2 inline comments as done.

Updated error handling and added the clang-format #include changes in.