[PATCH] D69822: [clang] Add new -fdebug-default-version flag.

[Paul Robinson via Phabricator via cfe-commits]

probinson added a comment.

The test is in the right place, now it needs to behave more like other driver tests.  Sorry if it feels like I'm whaling on you, but the driver is a bit of a peculiar beast with an atypical testing mode.  Taming it is harder than it looks.



================
Comment at: clang/include/clang/Driver/Options.td:1955
   Flags<[CC1Option]>;
+def fdwarf_default_version: Joined<["-"], "fdwarf-default-version=">, Group<f_Group>;
 def fdebug_prefix_map_EQ
----------------
Probably should have HelpText, maybe something like this:
"The default DWARF version to use, if a -g option causes DWARF debug info to be produced"
(it's probably not helpful to talk about overriding the target's default version, I think)


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6172
+  if (DwarfVersion == 0)
+    DwarfVersion = ParseDwarfDefaultVersion(getToolChain(), Args);
+
----------------
This is the path for parsing assembler options (when feeding clang a .s file) and I believe as written, the combination `-fdwarf-default-debug=2 -gdwarf-3` will yield an unused-option warning.   So I'm pretty sure the test doesn't cover this path.
You can fake an assembler driver test in the current test file by using `-x assembler` (especially if you use the `-###` tactic I mention in the comments there), or you can add a second test file with a .s extension. 


================
Comment at: clang/test/Driver/dwarf-default-version.c:1
+// RUN: %clang -target x86_64-linux-gnu -fdwarf-default-version=4 -gdwarf-2 -S -emit-llvm -o - %s | FileCheck %s --check-prefix=DWARF2
+// RUN: %clang -target x86_64-linux-gnu -gdwarf-3 -fdwarf-default-version=4 -S -emit-llvm -o - %s | FileCheck %s --check-prefix=DWARF3
----------------
As a rule, driver tests should pass `-###` which causes the driver to print the command lines instead of running the commands.  Then the checks would examine the command lines constructed by the driver, to verify they say what you want.
In this case, you'd look at the emitted `-dwarf-version` option and make sure it's what you want, and/or look for the option that says to do codeview.
The result is a test that is a more focused specifically on driver behavior, and also runs a bit faster (not starting any subprocesses etc).


================
Comment at: clang/test/Driver/dwarf-default-version.c:10
+// The -isysroot is used as a hack to avoid LIT messing with the SDKROOT
+// environment variable which indirecty overrides the version in the target
+// triple used here.
----------------
indirectly


================
Comment at: clang/test/Driver/dwarf-default-version.c:23
+// environment, we should use codeview. You can enable dwarf, which implicitly
+// disables codeview, of you can explicitly ask for both if you don't know how
+// the app will be debugged.
----------------
s/of/or/


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

https://reviews.llvm.org/D69822