This is an archive of the discontinued LLVM Phabricator instance.

[clang][cli] Generate and round-trip language options
ClosedPublic

Authored by jansvoboda11 on Feb 1 2021, 9:09 AM.

Details

Summary

This patch implements generation of remaining language options and tests it by performing parse-generate-parse round trip (on by default for assert builds, off otherwise).

This patch also correctly reports failures in parseSanitizerKinds, which is necessary for emitting diagnostics when an invalid sanitizer is passed to -fsanitize= during round-trip.

This patch also removes TableGen marshalling classes from two options:

  • fsanitize_blacklist When parsing: it's first initialized via the generated code, but then also changed by manually written code, which is confusing.
  • fopenmp When parsing: it's first initialized via generated code, but then conditionally changed by manually written code. This is also confusing. Moreover, we need to do some extra checks when generating it, which would be really cumbersome in TableGen. (Specifically, not emitting it when -fopenmp-simd was present.)

Diff Detail

Event Timeline

jansvoboda11 created this revision.Feb 1 2021, 9:09 AM
jansvoboda11 requested review of this revision.Feb 1 2021, 9:09 AM
Herald added a project: Restricted Project. · View Herald Transcript
jansvoboda11 edited the summary of this revision. (Show Details)Feb 1 2021, 9:34 AM
dexonsmith added inline comments.Feb 1 2021, 8:52 PM
clang/lib/Frontend/CompilerInvocation.cpp
1201

Can the caller use Diags.hasErrorOccurred() to avoid this change?

jansvoboda11 added inline comments.Feb 2 2021, 9:36 AM
clang/lib/Frontend/CompilerInvocation.cpp
1201

Not really, because the caller might have Diags that already contains some errors. We could compare the number of errors in Diags before and after calling this function. DiagnosticsEngine doesn't have API for that, but that could be added without much issues.

Do you have any specific reason for avoiding this change beyond minimizing the diff?

dexonsmith accepted this revision.Feb 2 2021, 11:02 AM

LGTM, since I don't think the parallel error paths are too bad in this specific patch, but see my longer comment inline.

clang/lib/Frontend/CompilerInvocation.cpp
1201

Not really, because the caller might have Diags that already contains some errors. We could compare the number of errors in Diags before and after calling this function. DiagnosticsEngine doesn't have API for that, but that could be added without much issues.

Why not just return "failure" at the top level if any error has occurred? Why do we need to know if a specific parse routine failed?

If we have top-level callers that send in a DiagnosticEngine that has already been called, maybe we can change them to call Reset before entering.

Do you have any specific reason for avoiding this change beyond minimizing the diff?

It's just fragile; it's a manual step to update Success every time an error is reported. Having two paths to give the same information is awkward and error-prone.

It'd be nice to have a hook that did both, so you could do something like:

reportError(diag::err_drv_invalid_value) << FlagName << Sanitizer;

and reportError would be something like:

auto reportError = [&Diags,&Success](OptSpecifier Opt) {
  Success = false;
  return Diags.Report(Opt);
};

or to ensure warnings in the same interface, maybe could have:

auto reportDiag = [&Diags,&Success](OptSpecifier Opt) {
  Success &= Diags.getDiagnosticLevel() >= DiagnosticsEngine::Error;
  return Diags.Report(Opt);
};

Not necessarily worth it for this patch (you can commit as-is), but if we're going to have to update swaths of code in this way it would be good to encapsulate the pattern somehow.

This revision is now accepted and ready to land.Feb 2 2021, 11:02 AM

Fix failures of tests that invoke -cc1 directly

Revert reporting failure when parsing sanitizers

Add comment, remove unnecessary diff

This revision was landed with ongoing or failed builds.Feb 9 2021, 1:19 AM
This revision was automatically updated to reflect the committed changes.