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

chandlerc · 2025-04-17T21:52:44Z

This lets us pass -no-disable-free to re-enable freeing memory for example. This is especially helpful for library users of Clang where it is important to not slowly leak memory.

llvmbot · 2025-04-17T21:53:18Z

@llvm/pr-subscribers-clang-driver

Author: Chandler Carruth (chandlerc)

Changes

This lets us pass -no-disable-free to re-enable freeing memory for example. This is especially helpful for library users of Clang where it is important to not slowly leak memory.

Full diff: https://github.com/llvm/llvm-project/pull/136213.diff

2 Files Affected:

(modified) clang/include/clang/Driver/Options.td (+7-3)
(modified) clang/test/Driver/clang-translation.c (+3)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index e9acb20348654..830d3459a1320 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -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">>;
 defm clear_ast_before_backend : BoolOption<"",
   "clear-ast-before-backend",
   CodeGenOpts<"ClearASTBeforeBackend">,
diff --git a/clang/test/Driver/clang-translation.c b/clang/test/Driver/clang-translation.c
index a7343ea18b213..1c9e3cfb2a5e5 100644
--- a/clang/test/Driver/clang-translation.c
+++ b/clang/test/Driver/clang-translation.c
@@ -10,6 +10,9 @@
 // I386: "-o"
 // I386: clang-translation
 
+// RUN: %clang -target i386-unknown-unknown -### -S -O0 -Os %s -o %t.s -fverbose-asm -fvisibility=hidden -no-disable-free 2>&1 | FileCheck -check-prefix=FREE %s
+// FREE-NOT: "-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"

llvmbot · 2025-04-17T21:53:18Z

@llvm/pr-subscribers-clang

Author: Chandler Carruth (chandlerc)

Changes

This lets us pass -no-disable-free to re-enable freeing memory for example. This is especially helpful for library users of Clang where it is important to not slowly leak memory.

Full diff: https://github.com/llvm/llvm-project/pull/136213.diff

2 Files Affected:

(modified) clang/include/clang/Driver/Options.td (+7-3)
(modified) clang/test/Driver/clang-translation.c (+3)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index e9acb20348654..830d3459a1320 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -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">>;
 defm clear_ast_before_backend : BoolOption<"",
   "clear-ast-before-backend",
   CodeGenOpts<"ClearASTBeforeBackend">,
diff --git a/clang/test/Driver/clang-translation.c b/clang/test/Driver/clang-translation.c
index a7343ea18b213..1c9e3cfb2a5e5 100644
--- a/clang/test/Driver/clang-translation.c
+++ b/clang/test/Driver/clang-translation.c
@@ -10,6 +10,9 @@
 // I386: "-o"
 // I386: clang-translation
 
+// RUN: %clang -target i386-unknown-unknown -### -S -O0 -Os %s -o %t.s -fverbose-asm -fvisibility=hidden -no-disable-free 2>&1 | FileCheck -check-prefix=FREE %s
+// FREE-NOT: "-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"

This lets us pass `-no-disable-free` to re-enable freeing memory for example. This is especially helpful for library users of Clang where it is important to not slowly leak memory.

zygoloid · 2025-04-17T23:27:21Z

clang/include/clang/Driver/Options.td

+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">>;


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 :)

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.

(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.)

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.

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

zygoloid · 2025-04-18T00:20:36Z

clang/include/clang/Driver/Options.td

+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">>;


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.

MaskRay · 2025-04-18T04:38:56Z

You might want to change the DisableFree default for library uses so that users don't need to specify -no-disable-free.

  // Clang calls BuryPointer on the internal AST and CodeGen-related elements like TargetMachine.
  // This will cause memory leaks if `compile` is executed many times.
  ci->getCodeGenOpts().DisableFree = false;
  ci->getFrontendOpts().DisableFree = false;

chandlerc · 2025-04-18T07:01:13Z

Follow-up PR as requested: #136271

chandlerc · 2025-04-18T17:34:11Z

You might want to change the DisableFree default for library uses so that users don't need to specify -no-disable-free.
  // Clang calls BuryPointer on the internal AST and CodeGen-related elements like TargetMachine.
  // This will cause memory leaks if `compile` is executed many times.
  ci->getCodeGenOpts().DisableFree = false;
  ci->getFrontendOpts().DisableFree = false;

Not really sure what default change you're thinking about?

The default that was causing me problems was the driver adding -disable-free to the CC1 flags by default -- but I think that's probably the correct default. I just wanted a way to override it in a library where the behavior would be problematic. Having the flag support appending -no-disable-free to the CC1 flags seemed like the cleanest solution that keeps the best defaults for the most common cases, but gives a path to handle the case of CC1 flags. Does that make sense?

This lets us pass `-no-disable-free` to re-enable freeing memory for example. This is especially helpful for library users of Clang where it is important to not slowly leak memory.

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Apr 17, 2025

Make the -disable-free flag more full featured

6c53f36

This lets us pass `-no-disable-free` to re-enable freeing memory for example. This is especially helpful for library users of Clang where it is important to not slowly leak memory.

chandlerc force-pushed the enable-free branch from 4e4ca5a to 6c53f36 Compare April 17, 2025 23:23

chandlerc requested a review from dwblaikie April 17, 2025 23:24

zygoloid reviewed Apr 17, 2025

View reviewed changes

zygoloid approved these changes Apr 18, 2025

View reviewed changes

chandlerc merged commit feb1fb5 into llvm:main Apr 18, 2025
9 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

chandlerc commented Apr 17, 2025

Uh oh!

llvmbot commented Apr 17, 2025

Uh oh!

llvmbot commented Apr 17, 2025

Uh oh!

zygoloid Apr 17, 2025

Uh oh!

chandlerc Apr 18, 2025

Uh oh!

chandlerc Apr 18, 2025

Uh oh!

zygoloid Apr 18, 2025

Uh oh!

chandlerc Apr 18, 2025

Uh oh!

zygoloid Apr 18, 2025

Uh oh!

Uh oh!

MaskRay commented Apr 18, 2025

Uh oh!

chandlerc commented Apr 18, 2025

Uh oh!

chandlerc commented Apr 18, 2025

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

chandlerc commented Apr 17, 2025

Uh oh!

llvmbot commented Apr 17, 2025

Uh oh!

llvmbot commented Apr 17, 2025

Uh oh!

zygoloid Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

chandlerc Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

chandlerc Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

zygoloid Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

chandlerc Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

zygoloid Apr 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaskRay commented Apr 18, 2025

Uh oh!

chandlerc commented Apr 18, 2025

Uh oh!

chandlerc commented Apr 18, 2025

Uh oh!

Uh oh!

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

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