1445522 - Add a static analysis to disallow `static const char* foo = "SomeStringLiteral";`

Status: NEW

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Masatoshi Kimura [:emk]]

It looks constant but actually it is not. The pointee is constant, but the pointer itself is mutable.

Adding another const will fix this:
  static const char* const foo = "SomeStringLiteral";

But it will still take an extra space in static storage although the compiler may optimize it out. So it is more desirable to use an array:
  static const char foo[] = "SomeStringLiteral";

We should have a static analysis to prevent such a waste.

Comment 1

[Andi [:andi]]

I did a test for this with clang 6.0 and generated the IR code. The program that I've build is something very trivial like:

>>static const char* var1 = "Some Data1";
>>static const char* const var2 = "Some Data2";
>>static const char var3[] = "Some Data3";
>>
>>int main() {
>>
>>  if (strcmp(var1, var2) + strcmp(var2, var3)+ strcmp(var1, var3)) return 1;
>>  return 0;
>>}

The correctness of the program is irrelevant here only I anted to see what is the storage class and size for each variable, and the IR was:

>>@_ZL4var1 = internal global i8* getelementptr inbounds ([11 x i8], [11 x i8]* @.str.1, i32 0, i32 0), align 8
>>@.str = private unnamed_addr constant [11 x i8] c"Some Data2\00", align 1
>>@_ZL4var3 = internal constant [11 x i8] c"Some Data3\00", align 1
>>@.str.1 = private unnamed_addr constant [11 x i8] c"Some Data1\00", align 1

Looking at all of the variables that we're intereed in they are all marked as constant, the only different is that |var3| is stored as a local symbol in the object file where the other two |var1| and |var2| will not be exposed to the object file directly, more then this if there are other symbols like |var1| and |var2| that are identical by string they can be merged, having the qualification |unnamed_addr|.
All of this was done with O0 as optimisation level, havign O1 or O2 would greatly diminish the impact, so I don't know what to say exactly about this. 
Ehsan what do you think about this?

Comment 2

[Masatoshi Kimura [:emk]]

`@_ZL4var1` is non-const and taking a space, no?

Although var2 is constant from the code generation perspective, it is still not a constant expression:

static const char* const var2 = "Some Data2";
static const char var3[] = "Some Data3";
constexpr static const char* var4 = var2;  // Error
constexpr static const char* var5 = var3;  // OK

Comment 3

[Andi [:andi]]

I agree with you on that one, I did a gross, not very filtered, searchfox in cpp and h files and the results are:
https://searchfox.org/mozilla-central/search?q=static%5Csconst%5Cschar%5Cs%3F%5C*(.*%5Cb)%5Cs%3F(%5C%3D)&case=false&regexp=true&path=.cpp
https://searchfox.org/mozilla-central/search?q=static%5Csconst%5Cschar%5Cs%3F%5C*(.*%5Cb)%5Cs%3F(%5C%3D)&case=false&regexp=true&path=.h

In total there are roughly  around 495 occurrences.

Comment 4

[(no longer active)]

I think Masatoshi-san is right and has found an interesting thing that we can easily find and filter out using a static analysis!

Any reason why you're looking at things at the LLVM level though as opposed to at the AST level?  I think it's more useful to look at the AST since that's the level at which we perform most of our static checks.  You can do that by passing -Xclang -ast-dump to clang.

Doing that with the initial test case gives:

|-VarDecl 0x55b627ea6910 <test.c:1:1, col:27> col:20 var1 'const char *' static cinit
| `-ImplicitCastExpr 0x55b627ef9460 <col:27> 'const char *' <BitCast>
|   `-ImplicitCastExpr 0x55b627ef9448 <col:27> 'char *' <ArrayToPointerDecay>
|     `-StringLiteral 0x55b627ef9410 <col:27> 'char [11]' lvalue "Some Data1"
|-VarDecl 0x55b627ef9490 <line:2:1, col:33> col:26 var2 'const char *const' static cinit
| `-ImplicitCastExpr 0x55b627ef9540 <col:33> 'const char *' <BitCast>
|   `-ImplicitCastExpr 0x55b627ef9528 <col:33> 'char *' <ArrayToPointerDecay>
|     `-StringLiteral 0x55b627ef94f0 <col:33> 'char [11]' lvalue "Some Data2"
`-VarDecl 0x55b627ef95d8 <line:3:1, col:28> col:19 var3 'const char [11]' static cinit
  `-StringLiteral 0x55b627ef9638 <col:28> 'const char [11]' lvalue "Some Data3"

In other words, in the static analysis you'd want to look for VarDecl's which have a pointer type which have an ArrayToPointerDecay underneath them.  It would be interesting to see how many of these things such an analysis would flag.  I would expect us to need to take care of a few more conditions that I can't think of off the top of my head but usually once you write the first analysis and run it on the tree you'll get some false positives which will tell you where you'd need to do more work.