CGDecl::emitStoresForConstant fix synthesized constant's name

[jfb]

Summary

The name of the synthesized constants for constant initialization was using mangling for statics, which isn't generally correct and (in a yet-uncommitted patch) causes the mangler to assert out because the static ends up trying to mangle function parameters and this makes no sense. Instead, mangle to "__const." + FunctionName + "." + DeclName.

[jfb]

I'm not sure this is the right fix because mangling confuses me. It fixes the assertion I'm encountering in my patch, and I don't think I can create a repro without the patch (since I'm asking to mangle a local in a way we don't seem to right now).

[erik.pilkington]

Can you give an example of some code that triggered this with your patch applied? Even if it isn't a real reproducer right now, it would help to try to understand whats happening here.

[jfb]

In D54055#1286196, @erik.pilkington wrote:

Can you give an example of some code that triggered this with your patch applied? Even if it isn't a real reproducer right now, it would help to try to understand whats happening here.

Sure!

I have code like this: getStaticDeclName(CGM, D) where D is vla in:

void foo(int size) {
  int vla[size];
  // ...
}

[rjmccall]

That sounds more like this use of the mangler isn't manipulating the function type context correctly. But actually I think the problem is that it's ridiculous to assume that arbitrary local declarations have meaningful manglings. Why are we calling getStaticDeclName on a variable that's obviously not static?

[jfb]

In D54055#1286396, @rjmccall wrote:

That sounds more like this use of the mangler isn't manipulating the function type context correctly. But actually I think the problem is that it's ridiculous to assume that arbitrary local declarations have meaningful manglings. Why are we calling getStaticDeclName on a variable that's obviously not static?

It was done in CodeGenFunction::EmitAutoVarInit a while ago. I moved it since then, but it's the same thing. I'm happy to mangle it any other way. At the end of the day we just need some name for an (unnamed address) global which is synthesized from a function-local initialization. We could just take the mangled function name and append something to it.

[rjmccall]

In D54055#1286397, @jfb wrote:

In D54055#1286396, @rjmccall wrote:

That sounds more like this use of the mangler isn't manipulating the function type context correctly. But actually I think the problem is that it's ridiculous to assume that arbitrary local declarations have meaningful manglings. Why are we calling getStaticDeclName on a variable that's obviously not static?

It was done in CodeGenFunction::EmitAutoVarInit a while ago. I moved it since then, but it's the same thing. I'm happy to mangle it any other way. At the end of the day we just need some name for an (unnamed address) global which is synthesized from a function-local initialization. We could just take the mangled function name and append something to it.

Okay. I assume this is internal-linkage and the name is just for debugging purposes? Maybe we could have an API to generate a best-effort name mangling that could intentionally punt on variably-modified types. (I'm not really sure why the type is being mangled here anyway.)

[jfb]

In D54055#1286514, @rjmccall wrote:

In D54055#1286397, @jfb wrote:

In D54055#1286396, @rjmccall wrote:

That sounds more like this use of the mangler isn't manipulating the function type context correctly. But actually I think the problem is that it's ridiculous to assume that arbitrary local declarations have meaningful manglings. Why are we calling getStaticDeclName on a variable that's obviously not static?

It was done in CodeGenFunction::EmitAutoVarInit a while ago. I moved it since then, but it's the same thing. I'm happy to mangle it any other way. At the end of the day we just need some name for an (unnamed address) global which is synthesized from a function-local initialization. We could just take the mangled function name and append something to it.

Okay. I assume this is internal-linkage and the name is just for debugging purposes? Maybe we could have an API to generate a best-effort name mangling that could intentionally punt on variably-modified types.

I can do that, if you think that's the right approach. Or I can use mangled function name + something (instead of trying to synthesize a static mangling). That seems pretty simple and it'll always work.

(I'm not really sure why the type is being mangled here anyway.)

Maybe the author of http://llvm.org/r127227 knows? :-)

[rjmccall]

Pfft, if I'm going to be held responsible for my own work, I'm in a lot of trouble. :)

Function name + local variable name WFM.

[jfb]

• As discussed on the review, change the constant generation to name the synthesized constant using '__const.' + function_name + '.' variable_name. The previous code wasn't generally correct because it was trying to mangle the variable as if it were a static, and in some cases this could pull in e.g. parameter names (which is nonsensical for a static). This only occurs with a patch which I haven't submitted yet, but my change is pretty well tested already as the modified tests show.

[jfb]

In D54055#1286523, @rjmccall wrote:

Pfft, if I'm going to be held responsible for my own work, I'm in a lot of trouble. :)

Function name + local variable name WFM.

Your wish is my command (in this specific instance anyways).

[jfb]

• clang-format

[rjmccall]

Seems reasonable to me.

[jfb]

• Fix lifetime of the name, Twine bites again.

[jfb]

Small update to use std::string because Twine lifetimes are sad. This didn't trigger on any of the tests, but did in a stage2 build.

[dexonsmith]

lib/CodeGen/CGDecl.cpp

990–998 ↗

(On Diff #174087)

Can you just kill Name entirely, throwing this inline, to avoid the malloc?

[jfb]

• Inline the name creating, save a heap allocation.

[jfb]

lib/CodeGen/CGDecl.cpp

990–998 ↗

(On Diff #174087)

Sure. It seems less readable, but since the function is already factored out it's not that bad.

lib/AST/ItaniumMangle.cpp

 /// this using 1-based ordinals.
 void CXXNameMangler::mangleFunctionParam(const ParmVarDecl *parm) {
   unsigned parmDepth = parm->getFunctionScopeDepth();
   unsigned parmIndex = parm->getFunctionScopeIndex();
 
   // Compute 'L'.
   // parmDepth does not include the declaring function prototype.
   // FunctionTypeDepth does account for that.
-  assert(parmDepth < FunctionTypeDepth.getDepth());
+  assert(parmDepth < FunctionTypeDepth.getDepth() ||
+         (parmDepth == FunctionTypeDepth.getDepth() &&
+          !FunctionTypeDepth.isInResultType()));
   unsigned nestingDepth = FunctionTypeDepth.getDepth() - parmDepth;
   if (FunctionTypeDepth.isInResultType())
     nestingDepth--;
 
   if (nestingDepth == 0) {
     Out << "fp";
   } else {
     Out << "fL" << (nestingDepth - 1) << 'p';