Style: Declaring constants as static

[Lambros Lambrou]

Hi all,

My C++ readability reviewer told me that compile-time constants should be declared as static, even when they are defined in function scope. Is this something we should be doing? The reviewer said that static constants are initialized at compile-time instead of run-time.

I understand that, if the constant is in an anonymous namespace, 'static' is implied, so we prefer not to add 'static' in that case (please confirm this).

However, my question is about constants that are declared inside a function, like this:

void DoStuff() {

  // Should this be 'static'?

  const int kThings = 42;

  // What about this case?

  const char* kFruits = {

    "apple",

    "banana"

  };

  // blah...

}

Note that the latter case isn't "technically" const - it's a non-const array of const char*. I could add another const, but that would violate the "do not go crazy with const" rule:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of_const#Use_of_const

So what effect, if any, does adding 'static' have in that case?

Thanks,

Lambros

[Thiago Farina]

const has internal linkage in C++, so:

foo.cc

#include "blah.h"

const int kFoo = 10; # internal to this unity. No need of static as in C.

But since people often need free functions inside source files as well
we usually wrap these constants in unnamed namespace, i.e:

foo.cc

#include "blah.h"

namespace {

const int kFoo = 10;

void MyAwesomeFunction() {
}

} // namespace

I'll let others comment about the use of 'static' inside functions
though, as I'm also curious about what they have to say.

--
Thiago

[Peter Kasting]

I'm going to bow out on the main question because I'm not certain.  

On Thu, Nov 15, 2012 at 10:10 AM, Lambros Lambrou <lambros...@chromium.org> wrote:

I understand that, if the constant is in an anonymous namespace, 'static' is implied, so we prefer not to add 'static' in that case (please confirm this).

File-scope "static" and an anonymous namespace are similar (in that they prevent a variable from being accessed outside the file) but not identical (in that static produces internal linkage while the namespace still results in external linkage).  There are esoteric consequences of this, like the fact that you can use the latter in template arguments but not the former, or that a debugger may be less confused with one or the other.  For most purposes, the distinctions don't really matter.  Generally people who prefer anonymous namespaces prefer them because "static" means too many things already and thus namespaces are less ambiguous; namespaces are already used for scoping in cases where they have names, so using unnamed ones is consistent; and putting "static" over and over on everything is verbose.

It used to be true that the standard officially deprecated the use of "static" for internal linkage, in favor of anonymous namespaces.  However, that has been reversed (see http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1012 ).

For constants specifically, in C++ (but not C), declaring a file-scope object as const gives it internal linkage by default, so it technically doesn't need to be "static" or in an anonymous namespace.  We don't generally make use of this because most people don't know it.

My recommendation is to use anonymous namespaces for this sort of thing, and not "static".  I think this provides maximum clarity.

 

  // What about this case?

  const char* kFruits = {

    "apple",

    "banana"

  };

Note that the latter case isn't "technically" const - it's a non-const array of const char*. I could add another const, but that would violate the "do not go crazy with const" rule:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of_const#Use_of_const

Your declaration is missing either a * or a [].  Assuming you declare as

const char* kFruits[] = { ... };

and not

const char** kFruits = { ... };

I think you're actually fine.  I don't think the former allows kFruits to be retargeted to point at another array.  The former is how we recommend people declare these sorts of things normally.

 

PK

[Lambros Lambrou]

On Thu, Nov 15, 2012 at 10:37 AM, Peter Kasting <pkas...@chromium.org> wrote:

Your declaration is missing either a * or a [].  Assuming you declare as

const char* kFruits[] = { ... };

Oops, yes, that's what I meant.

 

[Albert J. Wong (王重傑)]

On Thu, Nov 15, 2012 at 10:37 AM, Peter Kasting <pkas...@chromium.org> wrote:

I'm going to bow out on the main question because I'm not certain.  

On Thu, Nov 15, 2012 at 10:10 AM, Lambros Lambrou <lambros...@chromium.org> wrote:

I understand that, if the constant is in an anonymous namespace, 'static' is implied, so we prefer not to add 'static' in that case (please confirm this).

File-scope "static" and an anonymous namespace are similar (in that they prevent a variable from being accessed outside the file) but not identical (in that static produces internal linkage while the namespace still results in external linkage).  There are esoteric consequences of this, like the fact that you can use the latter in template arguments but not the former, or that a debugger may be less confused with one or the other.  For most purposes, the distinctions don't really matter.  Generally people who prefer anonymous namespaces prefer them because "static" means too many things already and thus namespaces are less ambiguous; namespaces are already used for scoping in cases where they have names, so using unnamed ones is consistent; and putting "static" over and over on everything is verbose.

It used to be true that the standard officially deprecated the use of "static" for internal linkage, in favor of anonymous namespaces.  However, that has been reversed (see http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#1012 ).

For constants specifically, in C++ (but not C), declaring a file-scope object as const gives it internal linkage by default, so it technically doesn't need to be "static" or in an anonymous namespace.  We don't generally make use of this because most people don't know it.

My recommendation is to use anonymous namespaces for this sort of thing, and not "static".  I think this provides maximum clarity.

Here are the specific spec sections talking about the linkage of const:

C++98 3.5.3:

   A name having namespace scope (3.3.5) has internal linkage if it is the name of

      — an object, reference, function or function template that is explicitly declared static or,

      — an object or reference that is explicitly declared const and neither explicitly declared extern nor

          previously declared to have external linkage; or

      — a data member of an anonymous union.

C++98 3.5.4:

   A name having namespace scope has external linkage if it is the name of

       — an object or reference, unless it has internal linkage; or

       — a function, unless it has internal linkage; or

       — a named class (clause 9), or an unnamed class defined in a typedef declaration in which the class has the

           typedef name for linkage purposes (7.1.3); or

       — a named enumeration (7.2), or an unnamed enumeration defined in a typedef declaration in which the

           enumeration has the typedef name for linkage purposes (7.1.3); or

       — an enumerator belonging to an enumeration with external linkage; or

       — a template, unless it is a function template that has internal linkage (clause 14); or

       — a namespace (7.3), unless it is declared within an unnamed namespace

Also 7.1.16:

    A name declared in a namespace scope without a storage-class-specifier has external linkage unless it has

   internal linkage because of a previous declaration and provided it is not declared const. Objects declared

   const and not explicitly declared extern have internal linkage.

So assuming I'm not reading it wrong, it seems to agree with Peter: global/namespace-scoped const things have internal linkage.  However, a function in a similar situation does not. Weird.

  // What about this case?

  const char* kFruits = {

    "apple",

    "banana"

  };

Note that the latter case isn't "technically" const - it's a non-const array of const char*. I could add another const, but that would violate the "do not go crazy with const" rule:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Use_of_const#Use_of_const

Your declaration is missing either a * or a [].  Assuming you declare as

const char* kFruits[] = { ... };

and not

const char** kFruits = { ... };

I think you're actually fine.  I don't think the former allows kFruits to be retargeted to point at another array.  The former is how we recommend people declare these sorts of things normally.

I actually think there is a difference here.  const inside a function scope doesn't internal or external linkage. By default, it has auto storage though so I think this requires the compile actually stuff kFruits onto the stack even though it's const.  I did a quite test program and disassembly and here's what I got:

void foo() {

  /* static */ const char* kFruits[] = {

    "apple",

    "banana"

  };

  for (int i=0; i < 2; ++i) {

    printf("%s\n", kFruits[i]);

  }

}

Without the static:

void foo() {

   0:   55                      push   %rbp

   1:   48 89 e5                mov    %rsp,%rbp

   4:   48 83 ec 20             sub    $0x20,%rsp

  const char* kFruits[] = {

    "apple",

    "banana"

  };

   8:   48 c7 45 e0 00 00 00    movq   $0x0,-0x20(%rbp)

   f:   00 

  10:   48 c7 45 e8 00 00 00    movq   $0x0,-0x18(%rbp)

  17:   00 

  for (int i=0; i < 2; ++i) {

  18:   c7 45 fc 00 00 00 00    movl   $0x0,-0x4(%rbp)

  1f:   eb 16                   jmp    37 <_Z3foov+0x37>

  --- snip --

with the static:

void foo() {

   0:   55                      push   %rbp

   1:   48 89 e5                mov    %rsp,%rbp

   4:   48 83 ec 10             sub    $0x10,%rsp

  static const char* kFruits[] = {

    "apple",

    "banana"

  };

  for (int i=0; i < 2; ++i) {

   8:   c7 45 fc 00 00 00 00    movl   $0x0,-0x4(%rbp)

   f:   eb 19                   jmp    2a <_Z3foov+0x2a>

  --- snip --

Note that rsp with static is decremented by 0x10 instead of 0x20 and we're missing some movqs.

-Albert

 

 

PK

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Peter Kasting]

On Thu, Nov 15, 2012 at 11:05 AM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

On Thu, Nov 15, 2012 at 10:37 AM, Peter Kasting <pkas...@chromium.org> wrote:

Your declaration is missing either a * or a [].  Assuming you declare as

const char* kFruits[] = { ... };

and not

const char** kFruits = { ... };

I think you're actually fine.  I don't think the former allows kFruits to be retargeted to point at another array.  The former is how we recommend people declare these sorts of things normally.

I actually think there is a difference here.  const inside a function scope doesn't internal or external linkage. By default, it has auto storage though so I think this requires the compile actually stuff kFruits onto the stack even though it's const.

I was not trying to answer the question of whether "static" makes a difference here or not; I was just trying to say that no extra "const" is necessary, or in fact possible, for the array case to be "fully const" -- it's already as const as it can get.  (Unless I'm mistaken.)

PK

[Albert J. Wong (王重傑)]

Oh, hah...sorry, I misread. Yeah, I agree const char* kFruits[] is const enough. Adding the last one (const char* const kFruits) is overkill IMO.

-Albert

 

PK

[Ami Fischman]

On Thu, Nov 15, 2012 at 10:10 AM, Lambros Lambrou <lambros...@chromium.org> wrote:

So what effect, if any, does adding 'static' have in that case?

Albert showed the effect - you avoid a bit of extra initialization of pointers at each invocation of DoStuff().

Clearly this wouldn't be worth spending time on as a perf optimization on its own.

But just as we prefer pre-increment to post-increment for loop int counters, even though there's no real perf impact, because it's better to be in the habit of pre-increment-by-default for more expensive ++ overloads, it's not unreasonable to prefer the pattern of static const here.

-a

[Peter Kasting]

On Thu, Nov 15, 2012 at 11:11 AM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

Oh, hah...sorry, I misread. Yeah, I agree const char* kFruits[] is const enough. Adding the last one (const char* const kFruits) is overkill IMO.

Actually, I was wrong.  There is a real difference between

const char* kFruits[]

and

const char* const kFruits[]

I recommend the latter.

For simple string constants, "const char kString[]" is probably best; "const char* const kString" is acceptable; "const char* kString" is generally not what you want.

I base these recommendations off an email from Mark Mentovai to this list in 2010.  He notes there that "const char* const" does not seem like excessive use of const per the style guide rule; I agree.  Since we're both readability reviewers, hopefully that's not too erroneous of guidance...

PK

[Brett Wilson]

On Thu, Nov 15, 2012 at 11:28 AM, Peter Kasting <pkas...@google.com> wrote:
> const char* const kFruits[]
>
> I recommend the latter.

I think this is overview and when I see this in a review, I request
removal of the second const.

Brett

[Jeffrey Yasskin]

On Thu, Nov 15, 2012 at 10:37 AM, Peter Kasting <pkas...@chromium.org> wrote:

allows one to write:

kFruits[0] = "pear";

If the compiler can see all of the uses of kFruits, and you don't actually write kFruits[0]=variable, then it'll optimize the same whether you use 'const' or not. However, if &kFruits escapes the translation unit, the compiler will lose some optimizations unless you declare it completely const:

$ cat test.cc
#include <string.h>
void escape(const void*);

long foo() {
  const char* MAYBE_CONST kFruits[] = {
    "apple",
    "banana"
  };
#ifdef ESCAPE
  escape(&kFruits);
#endif
  return strlen(kFruits[0]);
}

$ clang++ -DMAYBE_CONST= -O2 -S test.cc -o -
        .file   "test.cc"
        .text
        .globl  _Z3foov
        .align  16, 0x90
        .type   _Z3foov,@function
_Z3foov:                                # @_Z3foov
        .cfi_startproc
# BB#0:                                 # %entry
        movl    $5, %eax
        ret
.Ltmp0:
        .size   _Z3foov, .Ltmp0-_Z3foov
        .cfi_endproc


        .section        ".note.GNU-stack","",@progbits

$ clang++ -DMAYBE_CONST= -DESCAPE -O2 -S test.cc -o -
        .file   "test.cc"
        .text
        .globl  _Z3foov
        .align  16, 0x90
        .type   _Z3foov,@function
_Z3foov:                                # @_Z3foov
        .cfi_startproc
# BB#0:                                 # %entry
        subq    $24, %rsp
.Ltmp1:
        .cfi_def_cfa_offset 32
        movaps  .L_ZZ3foovE7kFruits(%rip), %xmm0
        movaps  %xmm0, (%rsp)
        leaq    (%rsp), %rdi
        callq   _Z6escapePKv
        movq    (%rsp), %rdi
        callq   strlen
        addq    $24, %rsp
        ret
.Ltmp2:
        .size   _Z3foov, .Ltmp2-_Z3foov
        .cfi_endproc

        .type   .L.str,@object          # @.str
        .section        .rodata.str1.1,"aMS",@progbits,1
.L.str:
        .asciz   "apple"
        .size   .L.str, 6

        .type   .L.str1,@object         # @.str1
.L.str1:
        .asciz   "banana"
        .size   .L.str1, 7

        .type   .L_ZZ3foovE7kFruits,@object # @_ZZ3foovE7kFruits
        .section        .rodata,"a",@progbits
        .align  16
.L_ZZ3foovE7kFruits:
        .quad   .L.str
        .quad   .L.str1
        .size   .L_ZZ3foovE7kFruits, 16


        .section        ".note.GNU-stack","",@progbits

$ clang++ -DMAYBE_CONST=const -DESCAPE -O2 -S test.cc -o -
        .file   "test.cc"
        .text
        .globl  _Z3foov
        .align  16, 0x90
        .type   _Z3foov,@function
_Z3foov:                                # @_Z3foov
        .cfi_startproc
# BB#0:                                 # %entry
        pushq   %rax
.Ltmp1:
        .cfi_def_cfa_offset 16
        movl    $_ZZ3foovE7kFruits, %edi
        callq   _Z6escapePKv
        movl    $5, %eax
        popq    %rdx
        ret
.Ltmp2:
        .size   _Z3foov, .Ltmp2-_Z3foov
        .cfi_endproc

        .type   _ZZ3foovE7kFruits,@object # @_ZZ3foovE7kFruits
        .section        .rodata,"a",@progbits
        .align  16
_ZZ3foovE7kFruits:
        .quad   .L.str
        .quad   .L.str1
        .size   _ZZ3foovE7kFruits, 16

        .type   .L.str,@object          # @.str
        .section        .rodata.str1.1,"aMS",@progbits,1
.L.str:
        .asciz   "apple"
        .size   .L.str, 6

        .type   .L.str1,@object         # @.str1
.L.str1:
        .asciz   "banana"
        .size   .L.str1, 7


        .section        ".note.GNU-stack","",@progbits

[Thiago Farina]

On Thu, Nov 15, 2012 at 5:31 PM, Brett Wilson <bre...@chromium.org> wrote:
> On Thu, Nov 15, 2012 at 11:28 AM, Peter Kasting <pkas...@google.com> wrote:
>> const char* const kFruits[]
>>

This says declares kFruits as an array of const pointer to const char.

>> I recommend the latter.
>
> I think this is overview and when I see this in a review, I request
> removal of the second const.
>

Why? Why wouldn't you want to make the pointer not const?

--
Thiago

[Mark Mentovai]

Lambros Lambrou wrote:

  // Should this be 'static'?

  const int kThings = 42;

Here, static doesn’t actually contribute anything*. Write “const int kThings;”. “static” is a no-op and a distraction.

  // What about this case?

  const char* kFruits = {

    "apple",

    "banana"

  };

Bottom line (repeated after the discussion): use “const char* const kFruits[]”.

There a few three possible ways of writing this:

“const char* kFruits[]”

One drawback here is that someone can come along and modify the pointers (but not the char data) by accidentally writing what amounts to “kFruits[0] = "cockroach";”. This results in subtle and hard-to-find bugs. I consider this a problem. This one is also generally the worst when it comes to wasting code and memory and run time, although compiler optimization can often (although not always) unravel lots of these and produce equivalent code. In fact, when I run Albert’s test program through the set of optimizations we use in release mode, it results in identical object code for each of the combinations I discuss here. The only real advantage to this one is that it’s succinct in the source code.

“static const char* kFruits[]”

This shares the problem with “const char* kFruits[]” that someone can modify a pointer. Now, the problem’s even worse: an accidentally-modified pointer is persistent, not just for the “activation” of the function in which it occurred, but for any subsequent activations as well. The problem’s been exacerbated, and the bugs in the program now may be even subtler and harder to find. Initialization here is usually fast because the initialized array and data will be present in the on-disk image and it’s paged in to memory already initialized.

“const char* const kFruits[]”

Along with preventing anyone from modifying the char data, the pointers are now sacred as well. You can’t modify the pointers, accident or otherwise, without resorting to heavy machinery. This is good defensive coding. Is the extra const “going overboard?” Generally, no, I don’t think it is. I think it’s fine and this is how I usually structure my own const char* arrays. Initialization here is just as fast as with “static const char* kFruits[]” for the same reason.

“static const char* const kFruits[]”

This doesn’t add anything relative to “const char* const kFruits[].” There’s no point in writing this*.

* I have assumed that the address of kThings and kFruits in all of these cases is irrelevant. You don’t care if &kThings or &kFruits evaluate to something different in distinct activations of your sample DoStuff() function, and you don’t care if &kThings or &kFruits remain valid after DoStuff() returns. This is a probably a reasonable assumption in light of this discussion being about constness and, more specifically, variables within the scope of a single function. In the rare case that you require a longer lifetime or that these pointers remain equivalent across distinct activations, take the advice I’m presenting here, but add “static” at the front of each. (Add the static, don’t replace a const with a static.)

Second-to-bottom-line: Use “const char* const kFruits[]”. If you were thinking of writing “static const char* kFruits”, get rid of the static and write a second const instead. It’s the same number of tokens, and it’s one character shorter. If you were thinking of just writing “const char* kFruits[]”, don’t, unless you have such a simple table used right next to something that’s obviously not going to be used in a way that might interfere with optimization (do you know what might interfere?) and so simple that it’s not going to be subject to accidental modification (can you guarantee this now and forever?)

Bottom line: Use “const char* const kFruits[]”.

[David Turner]

When working at removing static C++ initializers from the Chromium code, I discovered that function-scope static const objects are a really bad idea because often the compiler will generate *really* bad code for them. The typical example is the following (that real source code in Chromium, built using GCC 4.6):

A/ The code without a static const object:

void SomeClass::SomeMethod() {

   const TimeDelta kMaxDelay = TimeDelta::FromMilliseconds(100);

   if (this->cur_time - this->last_time > kMaxDelay) {

     // do stuff

   }

}

B/ The same code with a const object:

void SomeClass::SomeMethod() {

   static const TimeDelta kMaxDelay = TimeDelta::FromMilliseconds(100);

   if (this->cur_time - this->last_time > kMaxDelay) {

     // do stuff

   }

}

For A/, the compiler was able to completely optimize out the object and generate very tight machine code. I.e. something like this:

void SomeClass::SomeMethod() {

if (this->cur_time - this->last_time > 100e6) {

  // do stuff

   }

}

For B/, on the other hand, the compiler did the following:

- Allocate space for the kMaxDelay object in the (writable, non-shareable) .data section.
- Also allocate space for an additional 64-bit "guard variable".
- Add hidden lazy-initialization code at the start of SomeClass::SomeMethod(), accessing the data (through the GOT on Linux).
- At every use site, perform an actual memory load, instead of using the constant value wrapped into an object.

In short, the machine code implementation was doing this:

void SomeClass::SomeMethod() {

   void**      got = __get_got_address();

   TimeDelta*  __addr_kMaxDelay = got[__GOT_INDEX_OF_kMaxDelay];

   bool*       __addr_kMaxDelay__init = got[__GOT_INDEX_OF_kMaxDelay__init];

   if (*__addr_kMaxDelay_init == false) {
       *__addr_kMaxDelay__init = true;

       *__addr_kMaxDelay.value_ = 100e6;

   }

   if (this->cur_time.value_ - this->last_time.value_ > *__addr_kMaxDelay.value_)
   {

     // do stuff

   }

}

And that's because Chromium is compiled with -fno-threadsafe-statics, otherwise the pseudo-code generated by GCC would be even more complex, with call to C++ runtime calls to __cxa_guard_acquire/release/abort. Oh my.

As you can see, this is extremely inefficient. Moreovoer, this makes the method slower, and increases RAM usage for no good reason.

Note that all methods of TimeDelta involved here are completely inlined and really trivial. They _could_ have been optimized by a smarter compiler, this one simply decided not too, and it's a pretty recent one.

So my recommendation is, unless your constructor is very costly, never use static const objects with function scope. A const object seems fine though.

Also, as a reminder, defining const objects with file scope (static or not), generates a hidden C++ initializer that slows down loading your code in non-insignificant ways (especially on a cold start, because this implies additional page faults, often for stuff that is never going to be used before very long).

That's also why static const char kString[] = "......"; is always much better than static const std::string kString(".....");

My 2 euro-cents :-)

- Digit

--

[Peter Kasting]

On Thu, Nov 15, 2012 at 12:28 PM, David Turner <di...@chromium.org> wrote:

When working at removing static C++ initializers from the Chromium code, I discovered that function-scope static const objects are a really bad idea because often the compiler will generate *really* bad code for them.

Note that in your examples you use class objects.

The Google style guide basically bans static objects (at file or function scope) that are not POD types.  No one should ever be doing "static std::string ..." regardless of whether it's const.

If it's really necessary, one can use the CR_DEFINE_STATIC_LOCAL macro for function-scope statics of non-POD types to leak the object on shutdown.  This avoids static destruction, and since the object is function-scope and not file-scope, there is no static construction either (just construction the first time the function executes).  File-scope objects are still not OK because they still have to do static construction.

The previous advice in this thread should probably be read as applying to the POD case only.

PK

[Peter Kasting]

On Fri, Nov 16, 2012 at 5:05 AM, David Turner <di...@google.com> wrote:

On Thu, Nov 15, 2012 at 8:08 PM, Peter Kasting <pkas...@google.com> wrote:

I was not trying to answer the question of whether "static" makes a difference here or not; I was just trying to say that no extra "const" is necessary, or in fact possible, for the array case to be "fully const" -- it's already as const as it can get.  (Unless I'm mistaken.)

Actually, no, something like "const char* kFruits[] = { ... }" at file scope-level, still creates a mutable array of pointers in the .data section, and allows the code to assign values to kFruits[0], kFruits[1], etc...

If you use "const char* const kFruits[]" instead, the data will be placed in the .rodata section instead, so you won't be able to modify the pointers at runtime (even through a buffer overflow or random memory corruption), at least in theory (in practice, the last page of the .rodata section can still be kept writable by the dynamic linker due to page-alignment constraints).

Yeah, I corrected my mistake after that email :)

PK

[Dale Curtis]

To resurrect an old thread with some new context for caution... On Windows (at least (perhaps only in some cases)) function level statics which require run time initialization are initialized as:

static fancy_static = {};

bool have_set_static = false;

if (!have_set_static) {

  have_set_static = true;

  fancy_static = <blah blah blah>

}

Since have_set_static is set first, you end up with cases where concurrent calls may access the uninitialized static! Which leads to all sorts of fun such as http://crbug.com/179986 (fix). I'm tracking a cleanup for this in media/ here http://crbug.com/224662. If someone has some clang-fu / code search-fu to expose these Chrome wide, let me know.

To stay on topic: the style guide indicates POD statics aren't to be initialized with the result of functions at the global scope. Perhaps some text should be added to indicate that even function level scoped statics are not-thread safe? Or to take an extreme position, that they should be forbidden?

- dale

On Thu, Nov 15, 2012 at 12:28 PM, David Turner <di...@chromium.org> wrote:

[Ami Fischman]

FWIW, the canonical thread for "function-level static variable initialization is thread-unsafe in chrome" is https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/p6h3HC8Wro4/HHBMg7fYiMYJ (this applies to other platforms too, not just windows)

I agree it's worth calling out somewhere, esp. because this is a difference to google3 style (where different flags rule).

I wish chromium had a C++ Primer where this dire warning could go (mirroring go/oqols).

-a

--

[Daniel Cheng]

We could add a warning for this to the clang plugin (which of course wouldn't catch violations in Windows only files, which happens to be the only platform where it's unsafe...).

Daniel

[Bernhard Bauer]

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts ?

Bernhard.

[Mark Mentovai]

Because it’s unsafe on Windows, we build with -fno-threadsafe-statics on non-Windows to make it equally unsafe on all platforms.

That said, there are cases where you know that your code will only execute in a single thread, and it’s handy to write it using normal (non-thread-safe) statics instead of having to reach for a LazyInstance or something. I object to an outright ban on this form of static initialization.

[Ami Fischman]

Good point; done: http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static-variables.

[Peter Kasting]

On Thu, Mar 28, 2013 at 11:28 AM, Mark Mentovai <ma...@chromium.org> wrote:

That said, there are cases where you know that your code will only execute in a single thread, and it’s handy to write it using normal (non-thread-safe) statics instead of having to reach for a LazyInstance or something. I object to an outright ban on this form of static initialization.

To state this more strongly, the vast majority of Chromium code is, and should be, single-threaded.  (Even code that executes off the main threaded should usually be single-threaded and split different threads' functionality across separate objects that communicate via task posting or similar.)  Function-level statics are not the only problem in multi-threaded code (for example, as many threads have attested, thread-safe refcounting often leads to ownership confusion and bugs), and it needs to be very clear to developers when they're working with multi-threaded code so they can avoid these traps.

PK

[Victor Khimenko]

On Thu, Mar 28, 2013 at 10:28 PM, Mark Mentovai <ma...@chromium.org> wrote:

Because it’s unsafe on Windows, we build with -fno-threadsafe-statics on non-Windows to make it equally unsafe on all platforms.

This looks... like a weird idea. I mean: sure we should not use non-POD statics in portable parts of code and it's probably good idea not to rely on the fact that all platforms except Windows have thread-safe statics but why remove the safety net? Mistakes happen, sometimes people don't realize which statics are safe and which are not, etc.

[Peter Kasting]

On Thu, Mar 28, 2013 at 12:24 PM, Victor Khimenko <kh...@chromium.org> wrote:

On Thu, Mar 28, 2013 at 10:28 PM, Mark Mentovai <ma...@chromium.org> wrote:

Because it’s unsafe on Windows, we build with -fno-threadsafe-statics on non-Windows to make it equally unsafe on all platforms.

This looks... like a weird idea. I mean: sure we should not use non-POD statics in portable parts of code and it's probably good idea not to rely on the fact that all platforms except Windows have thread-safe statics but why remove the safety net? Mistakes happen, sometimes people don't realize which statics are safe and which are not, etc.

I would much rather have a "mistake" that results in unsafe code on one platform result in unsafe code on all platforms.  (Ideally, I'd love for it to crash immediately or even fail to compile.)  The "safety net" here doesn't make the browser safe, it just makes the bug harder to track down.

(Tangentially, given that 90+% of our users are on Windows, we really ought to be doing as much testing and development on Windows as possible.  The more that developers are doing all their design, implementation, and testing on non-Windows platforms, the more opportunities we have for platform differences to result in a suboptimal experience for most of our users.)

PK

[Mark Mentovai]

We don’t expect statics to behave thread-safely on any platform in our environment, so we write code that assumes they’re not thread-safe. Thread-safe statics aren’t free. Letting them be thread-safe on some platforms to add what you call a “safety net” inflates code size and causes a runtime performance penalty on those platforms. It also results in distinctions in interactions between threads on different platforms. I consider it a benefit that a mistake is unsafe on all platforms, because it means that Mac and Linux developers (and bots) are likely to catch the problem. That’s much better coverage than if we were just relying on Windows developers (and bots).

Maybe we would have come up with a different pattern if Windows wasn’t the first platform we brought up, but it doesn’t seem worthwhile to revisit this decision at this point.

[Dale Curtis]

To be clear: It's not just a matter of whether the code itself is multi-threaded or not but also if the code is used on multiple threads. E.g., a "single-threaded" class with statics used on multiple threads has the same problem even if each thread owns a copy of the class. The bug I linked above is an example of that case.

- dale

[Peter Kasting]

On Thu, Mar 28, 2013 at 12:47 PM, Dale Curtis <dalec...@chromium.org> wrote:

To be clear: It's not just a matter of whether the code itself is multi-threaded or not but also if the code is used on multiple threads. E.g., a "single-threaded" class with statics used on multiple threads has the same problem even if each thread owns a copy of the class. The bug I linked above is an example of that case.

OK-- to me that's multi-threaded code.  :)

This is another example of why our advice to developers has long been "do not use a class on more than one thread".  Using separate instances of the class for different threads may sound like a safe variant of that, but it's clearly not in this case.

PK 

[Peter Kasting]

On Thu, Mar 28, 2013 at 12:53 PM, Peter Kasting <pkas...@chromium.org> wrote:

This is another example of why our advice to developers has long been "do not use a class on more than one thread".  Using separate instances of the class for different threads may sound like a safe variant of that, but it's clearly not in this case.

It's worth clarifying: there's a difference between classes that actually do work and classes (or more likely structs) that just shuttle data around.  The latter are pretty clearly OK on multiple threads -- indeed, you often _have_ to use them that way, to communicate information between instances of the first type that are living on separate threads.  The advice here was only intended to apply to the more heavyweight "first type" classes (which are the kind likely to have static members anyway).

And of course, advice is just advice.  In different scenarios, breaking this can make sense.  (In the omnibox code I wrote, there's a heavyweight class that uses a single instance on multiple different threads, so I break this myself.)  It's more a "lean strongly toward this model, and break it only when you understand the consequences" kind of statement.

PK

[David Michael]

On Thu, Mar 28, 2013 at 12:55 PM, Ami Fischman <fisc...@chromium.org> wrote:

Good point; done: http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static-variables.

On Thu, Mar 28, 2013 at 11:28 AM, Bernhard Bauer <bau...@google.com> wrote:

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts ?

Maybe this is too obvious to state there, but using a static at all makes your code much more likely to be thread-unsafe. Maybe we should point out that it's best to avoid statics whenever practical.

[Peter Kasting]

On Thu, Mar 28, 2013 at 1:24 PM, David Michael <dmic...@chromium.org> wrote:

On Thu, Mar 28, 2013 at 12:55 PM, Ami Fischman <fisc...@chromium.org> wrote:

Good point; done: http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static-variables.

On Thu, Mar 28, 2013 at 11:28 AM, Bernhard Bauer <bau...@google.com> wrote:

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts ?

Maybe this is too obvious to state there, but using a static at all makes your code much more likely to be thread-unsafe. Maybe we should point out that it's best to avoid statics whenever practical.

No, it's not best to avoid statics wherever practical.  The style guide restricts usage of non-POD statics, and multi-threaded code should rarely use statics at all.  Statics outside those two realms are fine and adding a prohibition against them does not make sense.

PK

[Alpha Lam]

I imagine there's a similar problem with the SIMD code in color space conversion in media. Can we solve this problem by calling an initialize() method when the process starts? For example there is media::InitializeMediaLibrary() which gets called when the renderer starts. It can populate static structs the provide the correct function pointers after runtime detection of SIMD support. I know libvpx already does this.

Alpha

2013/3/28 Dale Curtis <dalec...@chromium.org>

[Gabriel Charette]

Reviving this thread once again with more data.

I just did some research with etienneb and we determined that for compound types (arrays, structs, unions, etc.) at function scope, static is required to avoid a copy at runtime.

From etienneb, here is an example using Visual Studio with -O2:

int dummy0(int x) {

  static const char A[] = "this is a test";

  return A[x];

}

int dummy1(int x) {

  const char A[] = "this is a test";

  return A[x];

}

int dummy2(int x) {

  const char* A = "this is a test";

  return A[x];

}

dummy0:

00401000 0F BE 80 10 78 40 00 movsx       eax,byte ptr A (407810h)[eax]  

00401007 C3                   ret

dummy1:

00401000 55                   push        ebp  

00401001 8B EC                mov         ebp,esp  

00401003 83 EC 10             sub         esp,10h  

00401006 A1 10 78 40 00       mov         eax,dword ptr [string "this is a test" (407810h)]  

0040100B 8B 0D 14 78 40 00    mov         ecx,dword ptr ds:[407814h]  

00401011 8B 15 18 78 40 00    mov         edx,dword ptr ds:[407818h]  

00401017 89 45 F0             mov         dword ptr [A],eax  

0040101A 66 A1 1C 78 40 00    mov         ax,word ptr ds:[0040781Ch]  

00401020 89 4D F4             mov         dword ptr [ebp-0Ch],ecx  

00401023 8A 0D 1E 78 40 00    mov         cl,byte ptr ds:[40781Eh]  

00401029 66 89 45 FC          mov         word ptr [ebp-4],ax  

0040102D 0F BE 45 F2          movsx       eax,byte ptr [ebp-0Eh]  

00401031 89 55 F8             mov         dword ptr [ebp-8],edx  

00401034 88 4D FE             mov         byte ptr [ebp-2],cl  

00401037 8B E5                mov         esp,ebp  

00401039 5D                   pop         ebp  

0040103A C3                   ret

dummy2:

00401040 0F BE 80 10 78 40 00 movsx       eax,byte ptr string "this is a test" (407810h)[eax]  

00401047 C3                   ret

The string itself is always placed in .rodata (read-only data section), but in the const char[] case it is copied onto the stack at the start of dummy1()...

The const char* case also achieves what we want, but in general we prefer to declare strings as const char[] in Chromium (also, here is an interesting article of why const char[] should always be prefered to const char*: http://glandium.org/blog/?p=2361).

The easiest is usually to simply declare a const char[] in an unnamed namespace (which, as the original poster mentioned, implies 'static' (in the C-sense of the keyword; e.g., local to file)).

But when local function scope is desired (which is often the case since our style-guide says to use the narrowest scope possible for variables), static should be used to prevent a copy of the string on the stack every time the method is called!

Note however that this only matters for compound types (arrays, structs, unions, etc.) and not for scalar types (int, bool, char, pointer) where the 'static' keyword is rather useless (but it's probably also harmless (perhaps it hurts inlining decisions?) FWIW I tried it quickly and both "static const int" and "const int" were inlined; so if it helps to make a simpler rule, we could also that all constants (compound or scalar) be made static when used at function scope..?).

I did a quick regex search through our codebase and it looks like the majority of function scope const arrays are not declared static as it is now:

(this finds any const [] declaration immediately following an open-curly-brace (that isn't the opening of a namespace) -- seems to be finding the right cases)

^((?!namespace).*)\{\n( )*const [a-z]* [a-zA-Z]*\[\] = .*;

253 matches across 91 files (this is not the full count, only the number of times this happens on the first line following an open-curly-brace)

^((?!namespace).*)\{\n( )*static const [a-z]* [a-zA-Z]*\[\] = .*;

77 matches across 42 files

I'm happy to put a CL together to fix this if we come to an agreement that this is the right thing to do.

Cheers!

Gab

[Thiago Farina]

On Wed, May 8, 2013 at 6:27 PM, Gabriel Charette <g...@chromium.org> wrote:
> The easiest is usually to simply declare a const char[] in an unnamed
> namespace (which, as the original poster mentioned, implies 'static' (in the
> C-sense of the keyword; e.g., local to file)).

We usually put it in unnamed namespace because we might also have
functions, enums, structs and classes at that level too. But
technically const by itself has internal linkage in C++.

--
Thiago

[Thiago Farina]

On Wed, May 8, 2013 at 6:27 PM, Gabriel Charette <g...@chromium.org> wrote:

> I'm happy to put a CL together to fix this if we come to an agreement that
> this is the right thing to do.
>

If we ever reach an agreement here, I think the decision should be put
in http://www.chromium.org/developers/coding-style/cpp-dos-and-donts

--
Thiago

[Avi Drissman]

And we'd better make the clang enforcer warn about it. This won't last if not enforced.

[Peter Kasting]

On Wed, May 8, 2013 at 2:27 PM, Gabriel Charette <g...@chromium.org> wrote:

I'm happy to put a CL together to fix this if we come to an agreement that this is the right thing to do.

I suspect people currently avoid static in these cases because they're worried it might result in a static initializer.  If we have a rule and a presubmit check to enforce static, and somewhere we explain that it won't cause static initializers, I think we're fine.

Tangentially, I would like to encourage people to declare constants and such within functions where they're used (if only used in one function), instead of atop a file (maybe in a namespace).  Doing this is IMO more compliant to the aforementioned style guide rule and is also easier to read and maintain.  I am currently cleaning up a lot of omnibox code that declares various unused constants at the file scope, and has difficult-to-debug functions which rely on lots of constants that are all defined far away in the file.  Both these problems are lessened when constants used in only one function are declared within that function.

PK

[Greg Thompson]

I agree wholeheartedly with the tangent. Keeping func-local constants in the func makes code easier to understand and audit.

--

[Thiemo Nagel]

[digging up an old thread]

The first sentence of the  "Static variables" section of C++ Dos and Don'ts now reads:

"Initialization of function-scope static variables is thread-unsafe in chromium, even on compilers/platforms such as gcc/linux where such idioms are normally safe."

As far as I've understood the discussion, that statement is a bit too strong. I'd suggest to add after "chromium": "(except for POD types that are initialized to compile-time constants)"

[Ami Fischman]

On Wed, Mar 12, 2014 at 3:27 AM, Thiemo Nagel <tna...@chromium.org> wrote:

except for POD types that are initialized to compile-time constants

What do you see as providing this guarantee?

-a

[Thiemo Nagel]

That's how I read § 3.6.2 of the standard:

Constant initialization is performed:

— if each full-expression (including implicit conversions) that appears in the initializer of a reference with

static or thread storage duration is a constant expression (5.19) and the reference is bound to an lvalue

designating an object with static storage duration or to a temporary (see 12.2);

— if an object with static or thread storage duration is initialized by a constructor call, if the constructor is

a constexpr constructor, if all constructor arguments are constant expressions (including conversions),

and if, after function invocation substitution (7.1.5), every constructor call and full-expression in

the mem-initializers and in the brace-or-equal-initializers for non-static data members is a constant

expression;

— if an object with static or thread storage duration is not initialized by a constructor call and if every

full-expression that appears in its initializer is a constant expression.

Together, zero-initialization and constant initialization are called static initialization; all other initial-

ization is dynamic initialization. Static initialization shall be performed before any dynamic initialization

takes place.

"Before any dynamic initialization takes place" to my mind implies "before any code is run".

[Ami Fischman]

I misunderstood what you meant previously.

 

"Before any dynamic initialization takes place" to my mind implies "before any code is run".

I wouldn't say that b/c e.g. .so's can be loaded after the process have been running for a while.  I think the relevant point is that any thread that can observe the value in question (without relying on implementation-defined behavior (e.g. 3.6.2-2-) will see the same value, so is safe.

I've updated the doc in question to be more explicit.  Please let me know if you have thoughts on it.  Thanks for the correction.

Cheers,

-a

[Peter Kasting]

On Wed, May 8, 2013 at 2:27 PM, Gabriel Charette <g...@chromium.org> wrote:

Reviving this thread once again with more data.

I just did some research with etienneb and we determined that for compound types (arrays, structs, unions, etc.) at function scope, static is required to avoid a copy at runtime.

This topic came up again recently.  Bumping this thread for two reasons:

(1) I forgot about it, maybe others did too.

(2) I retested the above on MSVC 2015 with constexpr and it still holds; this:

static constexpr char foo[]

Generates better code than this:

constexpr char foo[]

So, if declaring compound constexpr types on the stack, mark them static.

PK

[Alexander Yashkin]

Encountered just today link error with static const mebers of structures.
Building latest Chromium master on mac.

Sample code, reduced to minimum size:
struct MyStruct {
static const int value1 = 1;
static constexpr int value2 = 2;
};

void TestFunc() {
base::FundamentalValue* value = new
base::FundamentalValue(MyStruct::value1);
std::unique_ptr<base::FundamentalValue> value_ptr1(value);
auto value_ptr2 =
base::MakeUnique<base::FundamentalValue>(MyStruct::value1);
auto value_ptr3 =
base::MakeUnique<base::FundamentalValue>(MyStruct::value2);
}

Undefined symbols for architecture x86_64:
"extensions::MyStruct::value1", referenced from:
extensions::TestFunc() in libextensions.a(settings_overrides_api.o)
"extensions::MyStruct::value2", referenced from:
extensions::TestFunc() in libextensions.a(settings_overrides_api.o)

ld: symbol(s) not found for architecture x86_64


My GN configuration:
is_component_build = true
is_debug = true
enable_nacl = false

28.11.16 23:27, Peter Kasting пишет:

> On Wed, May 8, 2013 at 2:27 PM, Gabriel Charette <g...@chromium.org
> <mailto:g...@chromium.org>> wrote:
>
> Reviving this thread once again with more data.
>

> I just did some research with etienneb and we determined that *for
> compound types* (arrays, structs, unions, etc.) at function scope,
> *static is required to avoid a copy at runtime*.

>
>
> This topic came up again recently. Bumping this thread for two reasons:
> (1) I forgot about it, maybe others did too.
> (2) I retested the above on MSVC 2015 with constexpr and it still holds;
> this:
>
> static constexpr char foo[]
>
> Generates better code than this:
>
> constexpr char foo[]
>
> So, if declaring compound constexpr types on the stack, mark them static.
>
> PK
>

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

> ---
> You received this message because you are subscribed to the Google
> Groups "Chromium-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to chromium-dev...@chromium.org
> <mailto:chromium-dev...@chromium.org>.

--
С уважением,
Александр Яшкин,
разработчик Нашего Браузера
http://staff.yandex-team.ru/a-v-y/

[Peter Kasting]

On Wed, Nov 30, 2016 at 5:09 AM, Alexander Yashkin <a-...@yandex-team.ru> wrote:

Encountered just today link error with static const mebers of structures.
Building latest Chromium master on mac.

Sample code, reduced to minimum size:
struct MyStruct {
  static const int value1 = 1;
  static constexpr int value2 = 2;
};

These are different in several ways (not compound, struct members instead of stack locals).  When you declare static object members like this, and they are ODR-used, you need to give them storage.  This is a different issue entirely.

PK 

[Christian Biesinger]

Aren't you missing a definition for your constants?  I.e. a "int MyStruct::value1;" in a .cc file.

an email to chromium-dev+unsubscribe@chromium.org
<mailto:chromium-dev+unsubscrib...@chromium.org>.

--
С уважением,
Александр Яшкин,
разработчик Нашего Браузера
http://staff.yandex-team.ru/a-v-y/

[Alexander Yashkin]

Ok, got it, thanks. I used it in a way that requires definition.

http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#454

30.11.16 16:41, Peter Kasting пишет: