[PATCH] D146814: [Flang] Add debug flag to enable current debug information pass

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Thanks for the updates! A few more pointers, but nothing major.

Btw, are there any tests that check for debug info in the compiled files? For example:

  ! RUN: flang -g1 -S %s | FileCheck -check-prefixes=DEBUG-INFO-PRESENT
  ! RUN: flang -g0 -S %s | FileCheck -check-prefixes=DEBUG-INFO-MISSING
  
  end program



================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:32
+static void
+addFortranDebugOptions(ArgStringList &CmdArgs,
+                       llvm::codegenoptions::DebugInfoKind DebugInfoKind) {
----------------
There isn't anything Fortran specific here, is there? And this method basically implements https://github.com/llvm/llvm-project/blob/cf60d3f1a688671c8eb7859bf0572c403c3c0cca/clang/lib/Driver/ToolChains/Clang.cpp#L975-L1000, right? Why not extract it into e.g. `renderDebugEnablingArgs` and move to CommonArgs.cpp?


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:868
   parsePreprocessorArgs(res.getPreprocessorOpts(), args);
-  parseCodeGenArgs(res.getCodeGenOpts(), args, diags);
+  success &= parseCodeGenArgs(res.getCodeGenOpts(), args, diags);
   success &= parseSemaArgs(res, args, diags);
----------------
WDYT? I think that it makes sense to keep these separate.


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:144
+        val != llvm::codegenoptions::NoDebugInfo) {
+      // TODO: This is not a great warning message, could be improved
+      const auto debugErr = diags.getCustomDiagID(
----------------
SBallantyne wrote:
> awarzynski wrote:
> > Please improve it :)
> > 
> > In particular, you are testing for `DebugLineTablesOnly` and `NoDebugInfo`, yet the diagnostic refers to `-g1`.
> I previously had it emit these debug names, but i think its more confusing for the user as they will be passing `-g2 / -g3` in order to get this error, and the internal name is not as helpful. The TODO was from a previous patch and i just forgot to remove it, i am happy with the current state of warning. 
Thanks for the explanation! My main reservation is that it's not obvious how `-g2` and `-g3` map onto `llvm::codegenoptions`. This warnings refers to `-g1`, but there's no mention of `-g1` in this function. 

TBH, I would replace "Debug options greater than -g1 not yet implemented" with something a bit more generic and future-proof, e.g. `"Unsupported debug option: %0" << arg.getValue()`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146814/new/

https://reviews.llvm.org/D146814