Page MenuHomePhabricator

[flang][driver] Add `-fdebug-dump-parsing-log`
ClosedPublic

Authored by awarzynski on Feb 25 2021, 3:19 AM.

Details

Summary

Adds -fdebug-dump-parsing-log in the new driver. This option is
semantically identical to -fdebug-instrumented-parse in f18 (the
former is added as an alias in f18).

As dumping the parsing log makes only sense for instrumented parses, we
set Fortran::parser::Options::instrumentedParse to True when
-fdebug-dump-parsing-log is used. This is consistent with f18. To
facilate tweaking the frontend based on the action being run,
setUpFrontendBasedOnAction in CompilerInvocation.cpp is introduced.

Diff Detail

Event Timeline

awarzynski created this revision.Feb 25 2021, 3:19 AM
awarzynski requested review of this revision.Feb 25 2021, 3:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2021, 3:19 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
AMDChirag added inline comments.Feb 25 2021, 3:45 AM
flang/lib/Frontend/CompilerInvocation.cpp
88–95

Will this function be extended in the future?
If not, an entirely separate function for a couple statements seems rather overkill.

flang/lib/Frontend/FrontendActions.cpp
313–316

NIT: what's the point of these comments?
They are pretty much exactly the same as the function names.

Remove spurious comments

awarzynski marked an inline comment as done.Feb 25 2021, 7:15 AM
awarzynski added inline comments.
flang/lib/Frontend/CompilerInvocation.cpp
88–95

Will this function be extended in the future?

Yes.

Ideally we'd want _features_ and _actions_ to be orthogonal and to be controlled by dedicated flags. In this patch, we are adding an _action_ flag that toggles a _feature_ option. I think that that's a bit counter-intuitive, but not uncommon or unavoidable long-term.

Instead of adding comments, I prefer to introduce a dedicated method for this logic. It will make it easier for us to keep it in one place when new options like this are added. Also, ParseFrontendArgs is already quite long and is only going to get longer.

Having said all that, we may decide in the future that there's a better way to split this logic. For now I mostly want to avoid extending ParseFrontendArgs too much.

flang/lib/Frontend/FrontendActions.cpp
313–316

Good point, thank you!

AMDChirag added inline comments.Feb 25 2021, 7:26 AM
flang/lib/Frontend/CompilerInvocation.cpp
88–95

Fair, thank you for explaining!

In such case, shouldn't the function call setUpFrontendBasedOnAction(opts); be placed after all the fields of the opts variable have been set?

awarzynski updated this revision to Diff 327048.Mar 1 2021, 1:16 AM
awarzynski marked an inline comment as done.

Move the call to setUpFrontendBasedOnAction after all options have been set

awarzynski added inline comments.Mar 1 2021, 1:17 AM
flang/lib/Frontend/CompilerInvocation.cpp
88–95

Thanks for the suggestion, updated!

LGTM.
I'll let someone else review the patch if required.

It is unintuitive to me that we are mixing feature and action flags, however I don't have a better suggestion. LGTM.

awarzynski updated this revision to Diff 328970.Mar 8 2021, 4:19 AM

Generalise the test so that it's not architecture specific

I've just realised that the test was failing in Phabricator's CI. It turns out it's an X86-only failure (i.e. the test works fine on my AArch64 dev machine). This boils down to the order in which messages are printed. As the logs are unordered, one gets slightly different results on every platform. I extracted _one log_ (i.e. a block of N lines that's part of one log message) and used that for testing instead. Verified on both X86 and AArch64.

AMDChirag accepted this revision.Mar 9 2021, 3:48 AM
This revision is now accepted and ready to land.Mar 9 2021, 3:48 AM
This revision was landed with ongoing or failed builds.Mar 10 2021, 4:10 AM
This revision was automatically updated to reflect the committed changes.