On Fri, May 31, 2019 at 9:30 AM Martin Sebor <mse...@gmail.com> wrote:
>
> On 5/31/19 10:12 AM, Sean Gillespie wrote:
> > On Thu, May 30, 2019 at 4:28 PM Martin Sebor <mse...@gmail.com> wrote:
> >>
> >> On 5/28/19 3:30 PM, Sean Gillespie wrote:
> >>> This adds a new warning, -Wglobal-constructors, that warns whenever a
> >>> decl requires a global constructor or destructor. This new warning fires
> >>> whenever a thread_local or global variable is declared whose type has a
> >>> non-trivial constructor or destructor. When faced with both a constructor
> >>> and a destructor, the error message mentions the destructor and is only
> >>> fired once.
> >>>
> >>> This warning mirrors the Clang option -Wglobal-constructors, which warns
> >>> on the same thing. -Wglobal-constructors was present in Apple's GCC and
> >>> later made its way into Clang.
> >>>
> >>> Bootstrapped and regression-tested on x86-64 linux, new tests passing.
> >>
> >> I can't tell from the Clang online manual:
> >>
> >> Is the warning meant to trigger just for globals, or for all
> >> objects with static storage duration regardless of scope (i.e.,
> >> including namespace-scope objects and static locals and static
> >> class members)?
> >>
> >> "Requires a constructor to initialize" doesn't seem very clear
> >> to me. Is the warning intended to trigger for objects that
> >> require dynamic initialization? If so, then what about dynamic
> >> intialization of objects of trivial types, such as this:
> >>
> >> static int i = std::string ("foo").length ();
> >>
> >> or even
> >>
> >> static int j = strlen (getenv ("TMP"));
> >>
> >> If these aren't meant to be diagnosed then the description should
> >> make it clear (the first one involves a ctor, the second one does
> >> not). But I would think that if the goal is to find sources of
> >> dynamic initialization then diagnosing the above would be useful.
> >> If so, the description should make it clear and tests verifying
> >> that it works should be added.
> >>
> >> Martin
> >
> > The answer to both of those is yes. Clang warns on both of these with
> > -Wglobal-constructors because the goal of -Wglobal-constructors is to
> > catch and warn against potential life-before-main behavior.
> >
> > Based on Marek's feedback I'm about to push another patch that also
> > warns on your above two examples and adds tests for them. The idea
> > is that any C++-level dynamically initialized globals get warned. Constexpr
> > constructors or things that otherwise can be statically initialized
> > are not warned.
>
> That makes sense.
>
> >
> > Regarding the doc description, how about something like this:
> >
> > "Warns whenever an object with static storage duration requires
> > dynamic initialiation,
> > through global constructors or otherwise."
>
> This makes it clearer -- i.e, global constructors refers to the ELF
> concept rather than to the C++ one. What does the otherwise part
> refer to? Some non-ELF mechanism?
Ah, I was thinking "otherwise" to mean the C++ sense of constructor,
and not the ELF one. I realize now that it's clearer without "for
otherwise", since then "global constructors" would refer to just the
ELF concept.
>
> >> PS Dynamic initialization can be optimized into static
> >> initialization, even when it involves a user-defined constructor.
> >> If the purpose of the warning is to find objects that are
> >> dynamically initialized in the sense of the C++ language then
> >> implementing it in the front-end is sufficient. But if the goal
> >> is to detect constructors that will actually run at runtime (i.e.,
> >> have a startup cost and may have dependencies on one another) then
> >> it needs to be implemented much later.
> >>
> >
> > Clang sticks to the C++ language definition of dynamic initialization for
> > the
> > purposes of this warning and I think we should as well. Without runtime
> > checks
> > this is a best-effort warning, but it is capable of catching a bunch
> > of the common
> > ways that you can get constructors running on startup, which is the goal.
>
> It might be worth mentioning that in the manual, perhaps along with
> an example showing an instance of the warning that will most likely
> be a true positive vs a false positive. E.g., something like this:
>
> const char* const tmp = getenv ("TMP");
>
> vs
>
> const int n = strlen ("123"); // initialized statically
>
Will do.
Sean
