Skip to content

Make the -disable-free flag more full featured #136213

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 18, 2025
Merged

Make the -disable-free flag more full featured #136213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7889,9 +7889,13 @@ def code_completion_with_fixits : Flag<["-"], "code-completion-with-fixits">,
def skip_function_bodies : Flag<["-"], "skip-function-bodies">,
HelpText<"Skip function bodies when possible">,
MarshallingInfoFlag<FrontendOpts<"SkipFunctionBodies">>;
def disable_free : Flag<["-"], "disable-free">,
HelpText<"Disable freeing of memory on exit">,
MarshallingInfoFlag<FrontendOpts<"DisableFree">>;
defm disable_free : BoolOption<"",
"disable-free",
FrontendOpts<"DisableFree">,
DefaultFalse,
PosFlag<SetTrue, [], [ClangOption], "Disable">,
NegFlag<SetFalse, [], [ClangOption], "Don't disable">,
BothFlags<[], [ClangOption], " freeing of memory on exit">>;
Comment on lines +7892 to +7898
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is currently defined in a block of CC1-only options. If we want to expose this as a driver option, that should be done by moving it elsewhere in the file rather than by specifying [ClangOption] here. Though it's not clear to me if this is intended to be a driver option: ClangOption suggests yes, but the test using -Xclang suggests no.

If this is supposed to be a driver flag, those are usually namespaced by their initial letter, so I think it's worth considering if a namespace would make sense here too. If this only controls the behavior of the frontend then using a -f prefix might make sense, but I suppose we might also want to make this control the behavior of the driver itself, so maybe an unnamespaced flag is appropriate. GCC interprets -disable-free as enabling debugging dumps at phases i, s, a, b, ... of processing, so there's not a collision to worry about there.

The double-negative in -no-disable-free also seems a bit funny, but it's probably fine :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, this remains a CC1 flag (same as the below flag), and just allows us to invert it.

I wasn't suggesting lifting it all the way up, just making it have the more complete logical set of operations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I did check, and this remains rejected from the main driver, and only is allow at the CC1 layer, which works for my use case.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, it looks like the enclosing let Visibility = ... is entirely overriding the effect of the [ClangOption] here and below. So that's all just dead (and misleading) code. Great. I guess we should at least be consistent with it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll send a PR to at least remove the dead code as a follow-up.

defm clear_ast_before_backend : BoolOption<"",
"clear-ast-before-backend",
CodeGenOpts<"ClearASTBeforeBackend">,
Expand Down
4 changes: 4 additions & 0 deletions clang/test/Driver/clang-translation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
// I386: "-o"
// I386: clang-translation

// RUN: %clang -target i386-unknown-unknown -### -S %s -o %t.s -Xclang -no-disable-free 2>&1 | FileCheck -check-prefix=FREE %s
// FREE: "-disable-free"
// FREE: "-no-disable-free"

// RUN: %clang -target i386-unknown-unknown -### -S %s -fasynchronous-unwind-tables -fno-unwind-tables 2>&1 | FileCheck --check-prefix=UNWIND-TABLES %s --implicit-check-not=warning:
// UNWIND-TABLES: "-funwind-tables=2"

Expand Down
Loading