Page MenuHomePhabricator

[Flang] Exit gracefully with a useful message when we fail to lookup a target
ClosedPublic

Authored by mnadeem on Fri, Mar 17, 2:44 PM.

Details

Summary

Without this patch were asserting with a generic message Failed to create Target, but we already have a detailed error message stored in the variable error after calling lookupTarget() but this error was not getting used/printed.

With this patch we will emit a message with more details instead of a stack dump with a generic message.

Diff Detail

Event Timeline

mnadeem created this revision.Fri, Mar 17, 2:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Mar 17, 2:44 PM
Herald added a subscriber: sunshaoce. · View Herald Transcript
mnadeem requested review of this revision.Fri, Mar 17, 2:44 PM
mnadeem added inline comments.Fri, Mar 17, 2:46 PM
flang/lib/Frontend/FrontendActions.cpp
599

I tried

unsigned diagID = ci.getDiagnostics().getCustomDiagID(clang::
           DiagnosticsEngine::Fatal, "unable to create target: '%0'");

ci.getDiagnostics().Report(diagID) << error;

it prints the error but does not terminate flang-new invocation for some reason.

awarzynski added inline comments.Sat, Mar 18, 7:20 AM
flang/lib/Frontend/FrontendActions.cpp
598–600

You will need to update the function signature accordingly. Also, please add a test.

We already have an error message, so print it as well.

I'm not sure I follow - can you elaborate? This patch is switching from an assertion to a diagnostic. The error message inside the assert will be printed regardless of whether a diagnostic is used or not.

We already have an error message, so print it as well.

I'm not sure I follow - can you elaborate? This patch is switching from an assertion to a diagnostic. The error message inside the assert will be printed regardless of whether a diagnostic is used or not.

Before we were failing the assert with just a fixed message: "Failed to create Target".
With this patch we will also print the error description stored in the string error and then abort.

mnadeem updated this revision to Diff 506319.EditedSat, Mar 18, 1:50 PM
  • Use diagnostics (reused existing diag)
  • Add a test
mnadeem retitled this revision from [Flang] Replace assertion with fatal error while setting up target machine to [Flang] Exit gracefully with a useful message when we fail to lookup a target.Sat, Mar 18, 1:58 PM
mnadeem edited the summary of this revision. (Show Details)
mnadeem set the repository for this revision to rG LLVM Github Monorepo.Sat, Mar 18, 2:00 PM
mnadeem edited projects, added Restricted Project; removed Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptSat, Mar 18, 2:00 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript

We already have an error message, so print it as well.

I'm not sure I follow - can you elaborate? This patch is switching from an assertion to a diagnostic. The error message inside the assert will be printed regardless of whether a diagnostic is used or not.

Before we were failing the assert with just a fixed message: "Failed to create Target".
With this patch we will also print the error description stored in the string error and then abort.

Thanks for clarifying!

awarzynski accepted this revision.Sun, Mar 19, 5:24 AM

LGTM, thanks for improving this!

flang/test/Driver/target-machine-error.f90
1 ↗(On Diff #506319)

[nit] IMHO, it would be easier to understand the intended logic in this test when using some random string (e.g. some-invalid-triple) rather than deliberately misspelling a valid triple (e.g. aarch67-linux-gnu) .

This revision is now accepted and ready to land.Sun, Mar 19, 5:24 AM
tschuett added inline comments.
flang/lib/Frontend/FrontendActions.cpp
615

Do you need to add a check here as well?

mnadeem marked an inline comment as done.Mon, Mar 20, 12:20 PM
mnadeem added inline comments.
flang/lib/Frontend/FrontendActions.cpp
615

I briefly looked at the target code and it seems to me that this will only fail if the target does not support codgen or has not registered the target machine constructor. I think this is always expected to succeed.