-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[clang] Remove -g[no-]-* bool flags from g_Group #162750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There are four uses of BoolGOption, and all of them are essentially debug info feature flags, which I believe should not the enablement or disablement of all debug info emission. `OPT_g_group` is used to control the debug info level here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4387 This doesn't cause any test failures, and seems like the right behavior for all four flags: * -g[no-]flag-base * -g[no-]inline-line-tables * -g[no-]key-instructions * -g[no-]structor-decl-linkage-names None of these, even in the positive form, should enable debug info emission. Fixes llvm#162747
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Reid Kleckner (rnk) ChangesThere are four uses of BoolGOption, and all of them are essentially debug This doesn't cause any test failures, and seems like the right behavior
None of these, even in the positive form, should enable debug info Fixes #162747 Full diff: https://github.com/llvm/llvm-project/pull/162750.diff 3 Files Affected:
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index 9bfa1dd52effe..ced6f873516ba 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -545,15 +545,16 @@ multiclass BoolFOption<string flag_base, KeyPathAndMacro kpm,
Group<f_Group>;
}
-// Creates a BoolOption where both of the flags are prefixed with "g" and have
-// the Group<g_Group>.
+// Creates a BoolOption where both of the flags are prefixed with "g".
+// Does *not* map to g_Group, because that is reserved for flags that are
+// intended to enable (or disable) debug info, which is not appropriate for a
+// negative boolean flag (-gno-${feature}).
// Used for -cc1 frontend options. Driver-only options do not map to
// CompilerInvocation.
multiclass BoolGOption<string flag_base, KeyPathAndMacro kpm,
Default default, FlagDef flag1, FlagDef flag2,
BothFlags both = BothFlags<[]>> {
- defm NAME : BoolOption<"g", flag_base, kpm, default, flag1, flag2, both>,
- Group<g_Group>;
+ defm NAME : BoolOption<"g", flag_base, kpm, default, flag1, flag2, both>;
}
multiclass BoolMOption<string flag_base, KeyPathAndMacro kpm,
diff --git a/clang/test/DebugInfo/KeyInstructions/flag.cpp b/clang/test/DebugInfo/KeyInstructions/flag.cpp
index a5cd8558eae52..12802c1ccb823 100644
--- a/clang/test/DebugInfo/KeyInstructions/flag.cpp
+++ b/clang/test/DebugInfo/KeyInstructions/flag.cpp
@@ -1,5 +1,6 @@
// RUN: %clang -### -target x86_64 -c -gdwarf -gkey-instructions %s 2>&1 | FileCheck %s --check-prefixes=KEY-INSTRUCTIONS
// RUN: %clang -### -target x86_64 -c -gdwarf -gno-key-instructions %s 2>&1 | FileCheck %s --check-prefixes=NO-KEY-INSTRUCTIONS
+// RUN: %clang -### -target x86_64 -c -gno-key-instructions %s 2>&1 | FileCheck %s --check-prefixes=NO-DEBUG
//// Help.
// RUN %clang --help | FileCheck %s --check-prefix=HELP
@@ -7,6 +8,8 @@
// KEY-INSTRUCTIONS: "-gkey-instructions"
// NO-KEY-INSTRUCTIONS-NOT: key-instructions
+// NO-DEBUG-NOT: debug-info-kind
+// NO-DEBUG-NOT: dwarf
//// Help hidden: flag should not be visible.
// RUN: %clang --help | FileCheck %s --check-prefix=HELP
diff --git a/clang/test/Driver/debug-options.c b/clang/test/Driver/debug-options.c
index 73f2f402efa97..45ac450ac8faa 100644
--- a/clang/test/Driver/debug-options.c
+++ b/clang/test/Driver/debug-options.c
@@ -268,11 +268,11 @@
// RUN: %clang -### -c %s 2>&1 | FileCheck -check-prefix=NORNGBSE %s
// RUN: %clang -### -c -fdebug-ranges-base-address -fno-debug-ranges-base-address %s 2>&1 | FileCheck -check-prefix=NORNGBSE %s
//
-// RUN: %clang -### -c -gomit-unreferenced-methods -fno-standalone-debug %s 2>&1 | FileCheck -check-prefix=INCTYPES %s
+// RUN: %clang -### -c -g -gomit-unreferenced-methods -fno-standalone-debug %s 2>&1 | FileCheck -check-prefix=INCTYPES %s
// RUN: %clang -### -c %s 2>&1 | FileCheck -check-prefix=NOINCTYPES %s
-// RUN: %clang -### -c -gomit-unreferenced-methods -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 \
+// RUN: %clang -### -c -g -gomit-unreferenced-methods -fdebug-types-section -target x86_64-unknown-linux %s 2>&1 \
// RUN: | FileCheck -check-prefix=NOINCTYPES %s
-// RUN: %clang -### -c -gomit-unreferenced-methods -fstandalone-debug %s 2>&1 | FileCheck -check-prefix=NOINCTYPES %s
+// RUN: %clang -### -c -g -gomit-unreferenced-methods -fstandalone-debug %s 2>&1 | FileCheck -check-prefix=NOINCTYPES %s
//
// RUN: %clang -### -c -glldb %s 2>&1 | FileCheck -check-prefix=NOPUB %s
// RUN: %clang -### -c -glldb -gno-pubnames %s 2>&1 | FileCheck -check-prefix=NOPUB %s
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not enabling debug info for -gno-... options seems to be the right behavior. One might argue about the positive options, but I think, that's also correct. At least, it's better than the current situation: now the options being changed here override -gmlt
with -g
, which also doesn't seem intentional.
Looks good with one comment re: "Implies -g."
Default default, FlagDef flag1, FlagDef flag2, | ||
BothFlags both = BothFlags<[]>> { | ||
defm NAME : BoolOption<"g", flag_base, kpm, default, flag1, flag2, both>, | ||
Group<g_Group>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove the "Implies -g." part from the description of the affected options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
Thanks for the review! |
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/22599 Here is the relevant piece of the build log for the reference
|
There are four uses of BoolGOption, and all of them are essentially debug info feature flags, which I believe should not the enablement or disablement of all debug info emission. `OPT_g_group` is used to control the debug info level here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4387 This doesn't cause any test failures, and seems like the right behavior for all four flags: * -g[no-]flag-base * -g[no-]inline-line-tables * -g[no-]key-instructions * -g[no-]structor-decl-linkage-names None of these, even in the positive form, should enable debug info emission. Fixes llvm#162747
There are four uses of BoolGOption, and all of them are essentially debug info feature flags, which I believe should not the enablement or disablement of all debug info emission. `OPT_g_group` is used to control the debug info level here: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4387 This doesn't cause any test failures, and seems like the right behavior for all four flags: * -g[no-]flag-base * -g[no-]inline-line-tables * -g[no-]key-instructions * -g[no-]structor-decl-linkage-names None of these, even in the positive form, should enable debug info emission. Fixes llvm#162747
There are four uses of BoolGOption, and all of them are essentially debug
info feature flags, which I believe should not the enablement or
disablement of all debug info emission.
OPT_g_group
is used to controlthe debug info level here:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L4387
This doesn't cause any test failures, and seems like the right behavior
for all four flags:
None of these, even in the positive form, should enable debug info
emission.
Fixes #162747