Page MenuHomePhabricator

[flang][driver] Add options for -Werror
ClosedPublic

Authored by arnamoy10 on Mar 15 2021, 1:11 PM.

Details

Summary

Add the following option for the flang-driver

-Werror

With the option given, warnings are treated as error.

A test case is added.

Diff Detail

Event Timeline

arnamoy10 created this revision.Mar 15 2021, 1:11 PM
arnamoy10 requested review of this revision.Mar 15 2021, 1:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2021, 1:11 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
arnamoy10 updated this revision to Diff 331009.Mar 16 2021, 9:11 AM

Updating to use available option from Options.td instead of creating a new option for -Werror

Thank you for adding this @arnamoy10 !

I think that in order to match the semantics of -Werror from f18, this patch needs to be extended a bit. In the following two cases, f18 will exit immediately, whereas flang-new -fc1 will happily carry-on:

Could you try implementing similar semantics here?

I've left a few more comments inline.

flang/lib/Frontend/CompilerInvocation.cpp
352–355
  1. This will capture almost anything that matches -W<warning-flag>. Could you verify that it's indeed -Werror?
  2. This is a diagnostics rather than Sema option. We are likely to introduce much more with time. Could you move it to a separate method?
flang/test/Semantics/dosemantics03.f90
1–2 ↗(On Diff #331009)

Rather than adding the 2nd line, could you update the first line to use %flang_fc1 instead of %f18?

arnamoy10 added inline comments.Mar 23 2021, 9:12 AM
flang/test/Semantics/dosemantics03.f90
1–2 ↗(On Diff #331009)

I am not modifying this test case, as it would create a dependency on -std=f2018 up-streamed (due to this particular test case having dependency on language standard). I create a new test case, we can come back to it later.

arnamoy10 updated this revision to Diff 332692.Mar 23 2021, 9:14 AM

Thanks @awarzynski for the comments.

  1. Moving diagnostics options parsing to a separate function
  2. Adding a check so that -Wblah does not trigger the treatment of warnings as werrors, only -Werror triggers it
  3. Adding a new test case.
arnamoy10 edited the summary of this revision. (Show Details)Mar 23 2021, 9:40 AM
arnamoy10 updated this revision to Diff 332997.Mar 24 2021, 8:10 AM

rebasing on top of main

I see that -Werror changes the behavior of the driver in 5 different places. I would hope to see 5 new tests to verify each case.

flang/lib/Frontend/CompilerInvocation.cpp
353–354

Please update

flang/lib/Frontend/FrontendActions.cpp
67–70

Please add a test for this change.

102–105

Please add a test for this change.

116–119

Please add a test for this change.

273–276

Please add a test for this change.

flang/test/Driver/werror.f90
11–12 ↗(On Diff #332997)

Does this work for f18?

arnamoy10 updated this revision to Diff 333843.Mar 29 2021, 6:27 AM

Modifying the test cases to:

  1. Make it work for f18 (when flang-new is not installed)
  2. Adding more options and one test case to check correct functionality with PrescanAction and PrescanAndSemaAction

I really like how you split your tests into two files:

  • werror_scan.f captures warning generated by the prescanner
  • werror.f captures warnings from the semantic analysis

In every case you added multiple RUN lines to make sure that the behavior is consistent across multiple actions. I think that that's very useful. Ideally, we'd have one central switch for turning warnings into errors and this would be unnecessary. But we're not there yet. In the meantime, could you add a comment explaining why multiple RUN lines are used?

I have 2 more suggestions:

  • add yet another file for warnings from the parser
  • rename werror.f as `werror_sema.f

These are non-blockers for me.

flang/lib/Frontend/CompilerInvocation.cpp
360

AFAIK, no other option is allowed in -W<option>. Perhaps throw a diagnostic here?

flang/test/Driver/werror.f90
2 ↗(On Diff #333843)

[nit] This comment is not directly related to what's in this file (i.e. it doesn't really help understand it, does it)? I would remove it. Same in werror_scan.f.

4–6 ↗(On Diff #333843)

DELETEME

flang/test/Driver/werror_scan.f
6–8

DELETEME

10–12

This is going to be either flang-new -fc1 or f18. I think that we can start skipping this part of the comment to make it more generic.

arnamoy10 updated this revision to Diff 335246.EditedApr 5 2021, 7:04 AM
  1. Separated test cases to check prescan, parse and sema differently.
  2. Diagnostics is thrown when anything other that error is given for -W. This behavior is reflected in f18 as well.
  3. Minor changes to address other comments.
arnamoy10 updated this revision to Diff 335255.Apr 5 2021, 7:17 AM

Adding newline at the end of a test case.

awarzynski accepted this revision.Apr 5 2021, 7:32 AM

LGTM, thank you for addressing my comments!

This revision is now accepted and ready to land.Apr 5 2021, 7:32 AM
This revision was landed with ongoing or failed builds.Apr 5 2021, 9:49 AM
This revision was automatically updated to reflect the committed changes.

How about -Wl option, it will cause link error like " Only -Werror is supported currently"

How about -Wl option, it will cause link error like " Only -Werror is supported currently"

Thanks for pointing it out. The issue was due to restricting -W` options in f18 as well, this has been fixed with 4299ab6c5daf