Re: [PATCH] D35338: Add the -fdestroy-globals flag

[Vedant Kumar via cfe-commits]

Yeah, I think we have internal users who would be happy to use this flag as 
well.

Stepping back a bit. It's been a while since I followed the discussion on 
cfe-dev, but I don't recall there being any objections to the flag name or to 
using it for particular targets.

IIRC the objections are about deviating from the language standard. I'm not 
sure where I stand on the issue. I think if there's evidence -fdestroy-globals 
gives significant space savings, I'm happy to defer to users.

vedant

> On Jan 26, 2018, at 1:10 PM, Nico Weber via cfe-commits 
>  wrote:
> 
> I'd love to use this flag in non-firmware code FWIW.
> 
> On Fri, Jan 26, 2018 at 4:07 PM, Ian Tessier via Phabricator via cfe-commits 
> > wrote:
> itessier added a comment.
> 
> > That seems like a nice win and I like the convenience of this approach. 
> > That said I've just remembered that there's a thread on cfe-dev about this:
> > [RFC] Suppress C++ static destructor registration
> > I don't think a consensus was reached. From what I gather, some people 
> > think that the convenience of this flag makes it worth adding to clang, 
> > while others think that adding a non-standard compiler-specific flag is 
> > asking for trouble.
> 
> Given that firmware is a much different (or controlled) environment than a 
> binary running on a full blown OS, would it be acceptable to name the flag 
> -fbaremetal-destroy-globals, and only allow its use if the target triple's OS 
> is set to none (e.g.: arm-**none**-eabi)?
> 
> 
> https://reviews.llvm.org/D35338 
> 
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org 
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits 
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D35338: Add the -fdestroy-globals flag

[Nico Weber via cfe-commits]

I'd love to use this flag in non-firmware code FWIW.

On Fri, Jan 26, 2018 at 4:07 PM, Ian Tessier via Phabricator via
cfe-commits  wrote:

> itessier added a comment.
>
> > That seems like a nice win and I like the convenience of this approach.
> That said I've just remembered that there's a thread on cfe-dev about this:
> > [RFC] Suppress C++ static destructor registration
> > I don't think a consensus was reached. From what I gather, some people
> think that the convenience of this flag makes it worth adding to clang,
> while others think that adding a non-standard compiler-specific flag is
> asking for trouble.
>
> Given that firmware is a much different (or controlled) environment than a
> binary running on a full blown OS, would it be acceptable to name the flag
> -fbaremetal-destroy-globals, and only allow its use if the target triple's
> OS is set to none (e.g.: arm-**none**-eabi)?
>
>
> https://reviews.llvm.org/D35338
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35338: Add the -fdestroy-globals flag

[Ian Tessier via Phabricator via cfe-commits]

itessier added a comment.

> That seems like a nice win and I like the convenience of this approach. That 
> said I've just remembered that there's a thread on cfe-dev about this:
> [RFC] Suppress C++ static destructor registration
> I don't think a consensus was reached. From what I gather, some people think 
> that the convenience of this flag makes it worth adding to clang, while 
> others think that adding a non-standard compiler-specific flag is asking for 
> trouble.

Given that firmware is a much different (or controlled) environment than a 
binary running on a full blown OS, would it be acceptable to name the flag 
-fbaremetal-destroy-globals, and only allow its use if the target triple's OS 
is set to none (e.g.: arm-**none**-eabi)?


https://reviews.llvm.org/D35338



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35338: Add the -fdestroy-globals flag

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a subscriber: bruno.
vsk added a comment.

That seems like a nice win and I like the convenience of this approach. That 
said I've just remembered that there's a thread on cfe-dev about this:
[RFC] Suppress C++ static destructor registration

I don't think a consensus was reached. From what I gather, some people think 
that the convenience of this flag makes it worth adding to clang, while others 
think that adding a non-standard compiler-specific flag is asking for trouble.

I suspect that we'll need more consensus to get this in tree. See Richard's 
comment:
"If someone's prepared to write up a paper for the C++ committee exploring what 
it would mean to standardize this behaviour and actually fix the problem for 
everyone, it would seem extremely reasonable for clang trunk to carry a flag to 
enable that behaviour (and it certainly doesn't have to wait for the committee 
to respond)."


https://reviews.llvm.org/D35338



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35338: Add the -fdestroy-globals flag

[George Burgess IV via Phabricator via cfe-commits]

george.burgess.iv added a comment.

Thanks for working on this!

One small drive-by nit for you. Same "no need to update the diff this very 
second" as vsk; I can't LGTM this with confidence.

(Also, in the future, please try to include context 

 with your patch. Doing so makes reviewing it on Phab much easier. :) )




Comment at: include/clang/Frontend/CodeGenOptions.def:47
  ///< aliases to base ctors when possible.
+CODEGENOPT(DestroyGlobals, 1, 1) ///< -fdestroy-globals
 CODEGENOPT(DataSections  , 1, 0) ///< Set when -fdata-sections is enabled.

Nit: Please align the `///` with the above/below ones.


https://reviews.llvm.org/D35338



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35338: Add the -fdestroy-globals flag

[Ian Tessier via Phabricator via cfe-commits]

itessier added a comment.

In https://reviews.llvm.org/D35338#809146, @vsk wrote:

> This is interesting. Do you have any results/metrics to share (e.g some any 
> binary size reduction for projects you've looked at)?


I only tested this with Project Loon's avionics firmware which freed up ~1.2% 
of ROM space. A small amount, but for us every bit counts.

I did look at creating a templated wrapper class instead which uses 
placement-new to construct the wrapped type in a private char buffer. This 
results in the same effect as this patch (no dtor references are emitted). The 
only issue is the code is a big uglier to read, and we might forgot to use the 
wrapper (though a static checker could be added for this case). I figured it'd 
be best to add compiler support so other C++ based firmware projects can 
benefit as well.


https://reviews.llvm.org/D35338



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35338: Add the -fdestroy-globals flag

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

This is interesting. Do you have any results/metrics to share (e.g some any 
binary size reduction for projects you've looked at)?




Comment at: lib/Frontend/CompilerInvocation.cpp:585
   Opts.CXXCtorDtorAliases = Args.hasArg(OPT_mconstructor_aliases);
+  Opts.DestroyGlobals = !Args.hasArg(OPT_fno_destroy_globals);
   Opts.CodeModel = getCodeModel(Args, Diags);

I think this will do the wrong thing when passed: -cc1 -fno-destroy-globals 
-fdestroy-globals. Using hasFlag here should fix the issue. I'm just noting 
this as a FYI, there's no need to update the diff right away, since I'd first 
like to find out if there are any correctness concerns / what the use cases for 
this functionality are.


https://reviews.llvm.org/D35338



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35338: Add the -fdestroy-globals flag

[Ian Tessier via Phabricator via cfe-commits]

itessier created this revision.

The -fdestroy-globals flag can be used to disable global variable destructor
registration. It is intended to be used with embedded code that never exits.
Disabling registration allows the linker to garbage collect unused destructors
and vtables.


https://reviews.llvm.org/D35338

Files:
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/CGDeclCXX.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/Driver/fdestroy-globals.cpp

Index: test/Driver/fdestroy-globals.cpp
===
--- /dev/null
+++ test/Driver/fdestroy-globals.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang -fno-destroy-globals -### %s 2>&1 | FileCheck %s --check-prefix=CHECK-FLAG-DISABLE
+// RUN: %clang -fdestroy-globals -### %s 2>&1| FileCheck %s --check-prefix=CHECK-FLAG-ENABLE1
+// RUN: %clang -### %s 2>&1  | FileCheck %s --check-prefix=CHECK-FLAG-ENABLE2
+
+// RUN: %clang_cc1 -fno-destroy-globals -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-CODE-DISABLE
+// RUN: %clang_cc1 -fdestroy-globals -emit-llvm -o - %s| FileCheck %s --check-prefix=CHECK-CODE-ENABLE
+
+// CHECK-FLAG-DISABLE: "-cc1"
+// CHECK-FLAG-DISABLE: "-fno-destroy-globals"
+
+// CHECK-FLAG-ENABLE1: "-cc1"
+// CHECK-FLAG-ENABLE1-NOT: "-fno-destroy-globals"
+
+// CHECK-FLAG-ENABLE2: "-cc1"
+// CHECK-FLAG-ENABLE2-NOT: "-fno-destroy-globals"
+
+// CHECK-CODE-DISABLE-LABEL: define {{.*}} @__cxx_global_var_init
+// CHECK-CODE-DISABLE: call void @_ZN1AC1Ev{{.*}}
+// CHECK-CODE-DISABLE: ret void
+
+// CHECK-CODE-ENABLE-LABEL: define {{.*}} @__cxx_global_var_init
+// CHECK-CODE-ENABLE: call void @_ZN1AC1Ev{{.*}}
+// CHECK-CODE-ENABLE: %{{.*}} = call i32 @__cxa_atexit{{.*}}_ZN1AD1Ev
+// CHECK-CODE-ENABLE: ret void
+
+struct A {
+  virtual ~A() {}
+};
+
+A a;
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -582,6 +582,7 @@
   Opts.ObjCAutoRefCountExceptions = Args.hasArg(OPT_fobjc_arc_exceptions);
   Opts.CXAAtExit = !Args.hasArg(OPT_fno_use_cxa_atexit);
   Opts.CXXCtorDtorAliases = Args.hasArg(OPT_mconstructor_aliases);
+  Opts.DestroyGlobals = !Args.hasArg(OPT_fno_destroy_globals);
   Opts.CodeModel = getCodeModel(Args, Diags);
   Opts.DebugPass = Args.getLastArgValue(OPT_mdebug_pass);
   Opts.DisableFPElim =
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -3697,6 +3697,11 @@
   KernelOrKext)
 CmdArgs.push_back("-fno-use-cxa-atexit");
 
+  // -fdestroy-globals=1 is default.
+  if (!Args.hasFlag(options::OPT_fdestroy_globals,
+options::OPT_fno_destroy_globals, true))
+  CmdArgs.push_back("-fno-destroy-globals");
+
   // -fms-extensions=0 is default.
   if (Args.hasFlag(options::OPT_fms_extensions, options::OPT_fno_ms_extensions,
IsWindowsMSVC))
Index: lib/CodeGen/CGDeclCXX.cpp
===
--- lib/CodeGen/CGDeclCXX.cpp
+++ lib/CodeGen/CGDeclCXX.cpp
@@ -180,7 +180,7 @@
   EmitDeclInit(*this, D, DeclAddr);
 if (CGM.isTypeConstant(D.getType(), true))
   EmitDeclInvariant(*this, D, DeclPtr);
-else
+else if (CGM.getCodeGenOpts().DestroyGlobals)
   EmitDeclDestroy(*this, D, DeclAddr);
 return;
   }
Index: include/clang/Frontend/CodeGenOptions.def
===
--- include/clang/Frontend/CodeGenOptions.def
+++ include/clang/Frontend/CodeGenOptions.def
@@ -44,6 +44,7 @@
 CODEGENOPT(CXAAtExit , 1, 1) ///< Use __cxa_atexit for calling destructors.
 CODEGENOPT(CXXCtorDtorAliases, 1, 0) ///< Emit complete ctors/dtors as linker
  ///< aliases to base ctors when possible.
+CODEGENOPT(DestroyGlobals, 1, 1) ///< -fdestroy-globals
 CODEGENOPT(DataSections  , 1, 0) ///< Set when -fdata-sections is enabled.
 CODEGENOPT(UniqueSectionNames, 1, 1) ///< Set for -funique-section-names.
 CODEGENOPT(DisableFPElim , 1, 0) ///< Set when -fomit-frame-pointer is enabled.
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -1235,6 +1235,7 @@
   HelpText<"Don't use __cxa_atexit for calling destructors">;
 def fno_use_init_array : Flag<["-"], "fno-use-init-array">, Group, Flags<[CC1Option]>,
   HelpText<"Don't use .init_array instead of .ctors">;
+def fno_destroy_globals : Flag<["-"], "fno-destroy-globals">, Group, Flags<[CC1Option]>;
 def fno_unit_at_a_time : Flag<["-"], "fno-unit-at-a-time">, Group;
 def fno_unwind_tables : Flag<["-"], "fno-unwind-tables">, Group;
 def fno_verbose_asm :