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

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

What's the overall design goal here? 100% consistency with Clang? Could this be documented?



================
Comment at: clang/include/clang/Driver/ToolChain.h:23
 #include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/Triple.h"
 #include "llvm/Frontend/Debug/Options.h"
----------------
Unrelated change?


================
Comment at: clang/lib/Driver/ToolChains/Clang.h:16
 #include "clang/Driver/Types.h"
-#include "llvm/ADT/Triple.h"
 #include "llvm/Frontend/Debug/Options.h"
----------------
Unrelated change?


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:33
+// to the corresponding DebugInfoKind.
+static llvm::codegenoptions::DebugInfoKind DebugLevelToInfoKind(const Arg &A) {
+  assert(A.getOption().matches(options::OPT_gN_Group) &&
----------------
Looks identical to https://github.com/llvm/llvm-project/blob/cf60d3f1a688671c8eb7859bf0572c403c3c0cca/clang/lib/Driver/ToolChains/Clang.cpp#L406-L420. Why not move to https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/CommonArgs.cpp?


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:127
+    unsigned val =
+        llvm::StringSwitch<unsigned>(arg->getValue())
+            .Case("line-tables-only", llvm::codegenoptions::DebugLineTablesOnly)
----------------
1. Why `unsigned` instead of [[ https://github.com/llvm/llvm-project/blob/cf60d3f1a688671c8eb7859bf0572c403c3c0cca/clang/include/clang/Basic/DebugInfoOptions.h#L20-L55 | DebugInfoKind ]]
2. Why not use `std::optional`, e.g. `llvm::StringSwitch<std::optional<DebugInfoKind>>arg->getValue())`? This way you could avoid magic numbers like `~0U`.


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:137
+    if (val == ~0U) {
+      diags.Report(clang::diag::err_drv_invalid_value)
+          << arg->getAsString(args) << arg->getValue();
----------------
Please test this diagnostic. Also, if this is an error then you should return immediately and signal that this method has failed (e.g. `return false;`).


================
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(
----------------
Please improve it :)

In particular, you are testing for `DebugLineTablesOnly` and `NoDebugInfo`, yet the diagnostic refers to `-g1`.


================
Comment at: flang/lib/Frontend/CompilerInvocation.cpp:145
+      // TODO: This is not a great warning message, could be improved
+      const auto debugErr = diags.getCustomDiagID(
+          clang::DiagnosticsEngine::Warning,
----------------
Looks like you are creating a warning rather than an error: `debugErr` --> `debugWarn`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146814