[UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

[hoy]

Summary

C functions may be declared and defined in different prototypes like below. This patch unifies the checks for mangling names in symbol linkage name emission and debug linkage name emission so that the two names are consistent.

static int go(int);

static int go(a) int a;
{
  return a;
}

Test Plan:

[dblaikie]

Looks good - couple of minor/optional things.

clang/lib/CodeGen/CGDebugInfo.cpp

3525–3526

I'd probably roll this into the expression rather than adding another named variable - it doesn't seem to add much readability to me at least. Up to you.

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Does this need to be down here? Or would the code be a well exercised if it was up next to the go declaration above?

[tmsriram]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

18

Nice test, I didnt know you could do this!

[hoy]

clang/lib/CodeGen/CGDebugInfo.cpp

3525–3526

Good point, fixed.

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

18

I didn't know either. It's an old C usage. We hit this when compiling mysql.

34–39

Yes, it needs to be here. Otherwise it will just like the function bar above that doesn't get a uniquefied name. I think moving the definition up to right after the declaration hides the declaration.

[hoy]

Addresssing David's comment.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Not sure I follow - do you mean that if the go declaration and go definition were next to each other, this test would (mechanically speaking) not validate what the patch? Or that it would be less legible, but still mechanically correct?

I think it would be (assuming it's still mechanically correct) more legible to put the declaration next to the definition - the comment describes why the declaration is significant/why the definition is weird, and seeing all that together would be clearer to me than spreading it out/having to look further away to see what's going on.

[hoy]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

When the go declaration and go definition were next to each other, the go function won't get a uniqufied name at all. The declaration will be overwritten by the definition. Only when the declaration is seen by others, such the callsite in baz, the declaration makes a difference by having the callsite use a uniqufied name.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Ah! Interesting, good to know.

Is that worth supporting, I wonder? I guess it falls out for free/without significant additional complexity. I worry about the subtlety of the additional declaration changing the behavior here... might be a bit surprising/subtle. But maybe no nice way to avoid it either.

[hoy]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

It would be ideal if user never writes code like that. Unfortunately it exists with legacy code (such as mysql). I think it's worth supporting it from AutoFDO point of view to avoid a silent mismatch between debug linkage name and real linkage name.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Oh, I agree that we shouldn't mismatch debug info and the actual symbol name - what I meant was whether code like this should get mangled or not when using unique-internal-linkage names.

I'm now more curious about this:

When the go declaration and go definition were next to each other, the go function won't get a uniqufied name at all.

This doesn't seem to happen with the __attribute__((overloadable)) attribute, for instance - so any idea what's different about uniquification that's working differently than overloadable?

$ cat test.c
__attribute__((overloadable)) static int go(a) int a; {
  return 3 + a;
}
void baz() {
  go(2);
}
$ clang-tot test.c -emit-llvm -S -o - | grep go
  %call = call i32 @_ZL2goi(i32 2)
define internal i32 @_ZL2goi(i32 %a) #0 {

[hoy]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Good question. I'm not sure what's exactly going on but it looks like with the overloadable attribute, the old-style definition is treated as having prototype. But if you do this:

__attribute__((overloadable)) 
void baz() {}

then there's the error:

error: 'overloadable' function 'baz' must have a prototype
void baz() {

void baz() { does not come with a prototype. That's for sure. Sounds like int go(a) int a {; can have a prototype when it is loadable. I'm wondering why it's not always treated as having prototype, since the parameter type is there.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Yeah, that seems like that divergence be worth understanding (& if possible fixing/avoiding/merging). Ensuring these features don't have subtle divergence I think will be valuable to having a model that's easy to explain/understand/modify/etc.

[hoy]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

I took another look. I think the divergence comes from getAs<FunctionProtoType> vs hasPrototype. The debug data generation uses hasPrototype while getAs<FunctionProtoType> is used as overloadable attribute processing as long as unique linkage name processing before this change. More specifically, the following function definition is represented by FunctionProtoType while it does not hasPrototype.

static int go(a) int a; {
  return 3 + a;
}

I was trying to have CGDebugInfo to check FunctionProtoType instead of hasPrototype. While it works for the code pattern in discussion, it also breaks other tests including objectC tests. More investigation is needed to understand what each term really means.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Are you undertaking that investigation? It'd be good to address this divergence if possible.

(@aprantl or @rsmith maybe you know something about this ObjC thing? )

[hoy]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Haven't figured out anything useful yet. As far as I can tell, the debug info generation code is shared between C++ and ObjC. Using getAs<FunctionProtoType> works for C++ but not for ObjectC where it crashes when computing a mangled name for something like

void test() {
  __block A a;
  ^{ (void)a; };
}

Below are the failing tests which are all like that:

Clang :: CodeGenCXX/cp-blocks-linetables.cpp
Clang :: CodeGenCXX/debug-info-block-invocation-linkage-name.cpp
Clang :: CodeGenCXX/debug-info-blocks.cpp
Clang :: CodeGenObjCXX/nested-ehlocation.mm
Clang :: CodeGenObjCXX/property-objects.mm
Clang :: CodeGenObjCXX/synthesized-property-cleanup.mm

cc @bruno

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Ping on this - anyone got a chance to take a look? It'd be great to avoid this subtle inconsistency.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Ping again

[hoy]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

I tried a different route instead of using getAs<FunctionProtoType> in debug info generation which beaks the blocks function and objectC cases. Since the problem here is that the old-C function (bar in the test case) is not considered hasPrototype, I tried to unify isKNRPrototype and hasPrototype so that old-C functions are considered hasPrototype. It works for the name mangler but it breaks other places where isKNRPrototype should be excluded from hasPrototype.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Hrm - I'd really like to get to the bottom of this, but not sure who to pull in.

@aaron.ballman - do you know who might have some idea of how these different old KNR C declarations work, and how this code might be made more robust?

[aaron.ballman]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Ugh, prototypes. They're not particularly well specified in the C standard IMHO. In C, a function with a prototype is one that declares the types of its parameters (C17 6.2.1p2) which is further clarified to be a function type with a parameter type list explicitly (C17 6.2.7p3, 6.9.1p7). However, the very end of 6.9.1p7 goes on to say that once you see the definition of the function, you know about its parameter type information, but it doesn't clarify whether this means the function now has a prototype or not.

The result of this is that:

void f();
void call_it_once(void) { f(1.2f); }

void f(a) float a; {}
void call_if_twice(void) { f(1.2f); }

in call_it_once, the argument is promoted to a double, while in call_it_twice, the argument is not. I suspect we're hitting another variation of this confusing behavior here.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

@aaron.ballman Thanks for taking a look.

Do you know if/how this code could be phrased so that this code:

static int go(int);

void baz() {
  foo();
  bar(1);
  go(2);
}

static int go(a) int a;
{
  return glob + a;
}

Could test some property of go at the function call site that would be consistent whether the definition of go came before or after the call site? It seems currently this code behaves differently depending on that order and I think that's a bit of a sharp corner it'd be good not to have, if there's a tidier/more consistent way to phrase the code.

[aaron.ballman]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

To be honest, I wasn't aware this code was even valid (where you mix and match between identifier lists and parameter type lists), so that's neat.

I might be confused, but in my tests, the behavior of collectFunctionDeclProps() is the same regardless of order, but the calls to isUniqueInternalLinkageDecl() are different. Given an invocation of: -cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=4 -funique-internal-linkage-names -emit-llvm -o - test.c with test.c as:

static int go(int);

static int go(a) int a;
{
  return 1 + a;
}

void baz() {
  go(2);
}

I see isUniqueInternalLinkageDecl() gets called:

• once for go with no prototype
• once for baz with no prototype

and collectFunctionDeclProps() gets called:

• once for baz with no prototype
• once for go with a prototype

The end result is that I see !13 = distinct !DISubprogram(name: "go", scope: !8, file: !8, line: 3, type: !14, scopeLine: 4, flags: DIFlagPrototyped, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, retainedNodes: !2) in the output.

However, with test.c as:

static int go(int);

void baz() {
  go(2);
}

static int go(a) int a;
{
  return 1 + a;
}

I see isUniqueInternalLinkageDecl() gets called:

• once for baz with no prototype
• once for go with a prototype
• another one for go with a prototype

and collectFunctionDeclProps() gets called:

• once for baz with no prototype
• once for go with a prototype

The end result is that I see !13 = distinct !DISubprogram(name: "go", linkageName: "_ZL2goi.__uniq.39558841650144213141281977295187289852", scope: !8, file: !8, line: 7, type: !14, scopeLine: 8, flags: DIFlagPrototyped, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, retainedNodes: !2)

When I change isUniqueInternalLinkageDecl() to use !FD->getCanonicalDecl()->hasPrototype(), I get the same behavior with either ordering. When I run the full test suite with that change, I get no test failures, so that may be a reasonable fix worth investigating (I'm not super familiar with the ins and outs of name mangling and whether this change would be correct or not).

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Thanks so much, @aaron.ballman that does sound exactly like it summarizes the situation and the suggestion sounds like it could be the right thing.

@hoy does that all make sense to you/could you try @aaron.ballman's suggested change/fix?

[hoy]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Thanks for doing the experiment @aaron.ballman . Actually for the test in this diff, we would like the function bar to have a unique linkage name. The suggested change doesn't seem to fix that.

// bar should not be given a uniquefied name under -funique-internal-linkage-names, 
// since it doesn't come with valid prototype.
static int bar(a) int a;
{
  return glob + a;
}

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

I assume that comment was more written to describe existing practice than some intentional approach to dealing with a function like this, yeah?

The overloadable attribute seems to be able to mangle this function correctly - so I think that was my whole concern - that unique internal linkage names should, ideally, treat things the same as overloadable unless there's a reason these things are really different - which I don't know of any reason that they are.

So this looks like it fixes that gap - by enabling unique internal linkage names to mangle this case the same way overloadable does?

[hoy]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Sorry for not making it clear. The suggested fix does not mangle this case (function bar) while the overloadable attributes is able to mangle it.

[dblaikie]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Ah, right - thanks for clarifying/sorry for my misunderstanding!

[aaron.ballman]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

bar is never given a prototype, so I can see why it wouldn't mangle for you. Perhaps it mangles for overloadable because of ItaniumMangleContextImpl::shouldMangleCXXName() checking for the attribute and returning true?

[hoy]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Yes, and I think shouldMangleCXXName returns true because of FD->getType()->getAs<FunctionProtoType>() is true in the overloadable case. I guess I still don't fully understand the subtle diversion between FD->getType()->getAs<FunctionProtoType>() and FD->hasPrototype().

[aaron.ballman]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Yes, and I think shouldMangleCXXName returns true because of FD->getType()->getAs<FunctionProtoType>() is true in the overloadable case.

I don't think that's correct -- I think it returns true because of this: https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ItaniumMangle.cpp#L657

I guess I still don't fully understand the subtle diversion between FD->getType()->getAs<FunctionProtoType>() and FD->hasPrototype().

FD->getType()->getAs<FunctionProtoType>() is checking whether the given instance of the declaration has a type with a prototype. FD->hasPrototype() looks at whether the given instance has *or inherits* a prototype. So hasPrototype() is looking in more places for the prototype.

[hoy]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Thanks for pointing it out. I meant to say overloadable is only allowed on functions with getAs<FunctionProtoType>() but not hasPrototype: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L9825

On the contrary, isUniqueInternalLinkageDecl and CGDebugInfo rely on hasPrototype. This subtle diversion causes function bar to be mangled differently. Even with overloadable, function bar has a different symbol linkage name and debug linkage name:

_attribute__((overloadable))
static int bar(a) int a;
{
  return glob + a;
}

results in:

define internal i32 @_ZL3bari.__uniq.95397005697148547017947938379908873441(i32 %a)

but not a debug linkage name
!24 = distinct !DISubprogram(name: "bar", scope: !6, file: !6, line: 19, type: !25, scopeLine: 20, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !2, retainedNodes: !4)

[aaron.ballman]

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Thanks for pointing it out. I meant to say overloadable is only allowed on functions with getAs<FunctionProtoType>() but not hasPrototype: https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L9825

Ah, good to know -- I have no idea if that's intentional or not.

On the contrary, isUniqueInternalLinkageDecl and CGDebugInfo rely on hasPrototype. This subtle diversion causes function bar to be mangled differently. Even with overloadable, function bar has a different symbol linkage name and debug linkage name:

Unfortunately, it's a bit too far outside of my experience to know what the right answer is here. Sorry!

[dblaikie]

I took another look. I think the divergence comes from getAs<FunctionProtoType> vs hasPrototype. The debug data generation uses hasPrototype while getAs<FunctionProtoType> is used as overloadable attribute processing as long as unique linkage name processing before this change. More specifically, the following function definition is represented by FunctionProtoType while it does not hasPrototype.

Ah, sorry, maybe I'm coming around to this - so you're saying that the test in ItaniumMangleContextImpl::isUniqueInternalLinkageDecl must match the check in CGDebugInfo::collectFunctionDeclProps And when they diverge something bad happens? (could you refresh me on what that breaks - something crashes in ObjectiveC test cases? Or the tests fail?)

I wonder whether we should change both of them then?

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

34–39

Thanks for taking a look, @aaron.ballman - maybe we can get @rsmith in here at some point...

[hoy]

In D98799#2757959, @dblaikie wrote:

I took another look. I think the divergence comes from getAs<FunctionProtoType> vs hasPrototype. The debug data generation uses hasPrototype while getAs<FunctionProtoType> is used as overloadable attribute processing as long as unique linkage name processing before this change. More specifically, the following function definition is represented by FunctionProtoType while it does not hasPrototype.

Ah, sorry, maybe I'm coming around to this - so you're saying that the test in ItaniumMangleContextImpl::isUniqueInternalLinkageDecl must match the check in CGDebugInfo::collectFunctionDeclProps And when they diverge something bad happens? (could you refresh me on what that breaks - something crashes in ObjectiveC test cases? Or the tests fail?)

I wonder whether we should change both of them then?

Yes, from AutoFDO point of view they should match, since both ELF symbol names and debug names are used to generate profiles. The current patch unifies them but it causes divergence from overloadable. Previously, ItaniumMangleContextImpl::isUniqueInternalLinkageDecl was consistent with overloadable. I tried to unify in the other way so that all the three places are consistent, i.e, using FD->getType()->getAs<FunctionProtoType>() everywhere and it caused the following tests to fail:

Clang :: CodeGenCXX/cp-blocks-linetables.cpp
Clang :: CodeGenCXX/debug-info-block-invocation-linkage-name.cpp
Clang :: CodeGenCXX/debug-info-blocks.cpp
Clang :: CodeGenObjCXX/nested-ehlocation.mm
Clang :: CodeGenObjCXX/property-objects.mm
Clang :: CodeGenObjCXX/synthesized-property-cleanup.mm

[dblaikie]

OK - poked around a bit more to better understand this, so attempting to summarize my current understanding (excuse the repetition from previous parts of this thread) and results.

• __attribute__((overloadable)) can/does mangle K&R C style declarations
• with this patch (currently committed), -funique-internal-linkage-names does not mangle K&R C style declarations (see bar in the included test case, unmangled)
• I'd like to avoid that divergence if possible
• Changing the debug info code to be more generous with names it mangles (by using FD->getType()->getAs<FunctionProtoType>() rather than hasPrototype()) causes problems
  • Specifically: Objective C blocks (which have a FunctionProtoType type, but !hasPrototype it seems) are missing parameter info so this call crashes
    • There doesn't seem to be any way to test for this property of the FunctionDecl that I can see - where it has a type, but doesn't have parameter info

Trying to pull in some folks who might know what's going on here/be able to suggest a way to split these cases if needed, or fix the block FunctionDecls to have param info. @rjmccall @JDevlieghere - I'd really appreciate some help here.

[rjmccall]

In D98799#2761684, @dblaikie wrote:

OK - poked around a bit more to better understand this, so attempting to summarize my current understanding (excuse the repetition from previous parts of this thread) and results.

• __attribute__((overloadable)) can/does mangle K&R C style declarations
• with this patch (currently committed), -funique-internal-linkage-names does not mangle K&R C style declarations (see bar in the included test case, unmangled)
• I'd like to avoid that divergence if possible
• Changing the debug info code to be more generous with names it mangles (by using FD->getType()->getAs<FunctionProtoType>() rather than hasPrototype()) causes problems
  • Specifically: Objective C blocks (which have a FunctionProtoType type, but !hasPrototype it seems) are missing parameter info so this call crashes
    • There doesn't seem to be any way to test for this property of the FunctionDecl that I can see - where it has a type, but doesn't have parameter info

Trying to pull in some folks who might know what's going on here/be able to suggest a way to split these cases if needed, or fix the block FunctionDecls to have param info. @rjmccall @JDevlieghere - I'd really appreciate some help here.

Unlike lambdas, BlockDecl is not a subclass of FunctionDecl, so I'm not quite sure what you're asking — there shouldn't be a way for that line to crash on a BlockDecl because FD should be null.

Not sure what you mean by block FunctionDecl — a block doesn't make a FunctionDecl. If it's useful for BlockDecl to return

[dblaikie]

Thanks for taking a look, @rjmccall - I really appreciate it!

Sorry I'm doing a bad job describing (admittedly I'm pretty confused about all this, which is most of the issue) the issue - I'll try to clarify as best I can.

In D98799#2781200, @rjmccall wrote:

In D98799#2761684, @dblaikie wrote:

OK - poked around a bit more to better understand this, so attempting to summarize my current understanding (excuse the repetition from previous parts of this thread) and results.

• __attribute__((overloadable)) can/does mangle K&R C style declarations
• with this patch (currently committed), -funique-internal-linkage-names does not mangle K&R C style declarations (see bar in the included test case, unmangled)
• I'd like to avoid that divergence if possible
• Changing the debug info code to be more generous with names it mangles (by using FD->getType()->getAs<FunctionProtoType>() rather than hasPrototype()) causes problems
  • Specifically: Objective C blocks (which have a FunctionProtoType type, but !hasPrototype it seems) are missing parameter info so this call crashes
    • There doesn't seem to be any way to test for this property of the FunctionDecl that I can see - where it has a type, but doesn't have parameter info

Trying to pull in some folks who might know what's going on here/be able to suggest a way to split these cases if needed, or fix the block FunctionDecls to have param info. @rjmccall @JDevlieghere - I'd really appreciate some help here.

Unlike lambdas, BlockDecl is not a subclass of FunctionDecl, so I'm not quite sure what you're asking — there shouldn't be a way for that line to crash on a BlockDecl because FD should be null.

Not sure what you mean by block FunctionDecl — a block doesn't make a FunctionDecl. If it's useful for BlockDecl to return

Did this ^ get cut off in some way ("If it's useful for BlockDecl to return" ... something?).

Anyway - so this crash only seems to happen in Objective C++ tests. It's not the block itself, but I'm guessing some implementation function, specifically:

FunctionDecl 0xf921248 <<invalid sloc>> <invalid sloc> __Block_byref_object_copy_ 'void (void *, void *)' static <<<NULL params x 2>>>

This FunctionDecl's type is a FunctionProtoType, but doesn't have any parameter type info, which seems weird. Maybe that's the bug - perhaps whatever creates this should be adding type info like for other FunctionProtoTyped FunctionDecls?

Ah, here it is: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBlocks.cpp#L1958 - that creates this seemingly weird FunctionDecl that has a prototype type, but if you try to query the type of its parameters, it'll crash.

Other FunctionDecls get their ParmVarDecls in places like this: https://github.com/llvm/llvm-project/blob/7daa18215905c831e130c7542f17619e9d936dfc/clang/lib/Sema/SemaDecl.cpp#L9478 and look like this:

FunctionDecl 0xf923bb0 <test.cpp:1:1, col:42> col:42 go 'int (int)' static
`-ParmVarDecl 0xf923ae8 <col:45, col:49> col:49 a 'int'

Should the synthetic FunctionDecl created by CGBlocks.cpp be changed to be more fully featured and have valid ParmVarDecls?

[aside: Looks like the bug here that I'm interested in sort of already exists apart from the unique linkage name case here - we don't add the mangled name of __attribute__((overloadable)) to DWARF even though we should, and if I try to fix that the obvious way (testing FD->getType()->getAs<FunctionProtoType>() instead of FD->hasPrototype() in collectFunctionDeclProps then it crashes the ObjC++ block tests for the above reason]

[rjmccall]

Hmm. CC'ing Akira. Akira, do you know why we're building a fake FunctionDecl as part of emitting these helper functions? Is it something we can avoid?

[ahatanak]

I don't know know why these fake FunctionDecls are needed, but it looks like it's okay to avoid creating them. I see a few debug info tests failing if GlobalDecl() instead of a fake function is passed to StartFunction, but it looks like the test check strings should be changed.

AFAICT CodeGenFunction::GenerateCopyHelperFunction has been creating these fake FunctionDecls from the beginning and subsequent patches didn't fix it:

https://github.com/llvm/llvm-project/commit/0c74327715af823930cb583490d315f64ef8de4e

[dblaikie]

In D98799#2794366, @ahatanak wrote:

I don't know know why these fake FunctionDecls are needed, but it looks like it's okay to avoid creating them. I see a few debug info tests failing if GlobalDecl() instead of a fake function is passed to StartFunction, but it looks like the test check strings should be changed.

AFAICT CodeGenFunction::GenerateCopyHelperFunction has been creating these fake FunctionDecls from the beginning and subsequent patches didn't fix it:

https://github.com/llvm/llvm-project/commit/0c74327715af823930cb583490d315f64ef8de4e

Would you be able to make that change, if it's not too much work? (I'm not as familiar with the Objective C(++) stuff)

[ahatanak]

Yes, I think I can fix that.

[dblaikie]

In D98799#2794546, @ahatanak wrote:

Yes, I think I can fix that.

Thanks a lot! If you could follow-up here when that's committed, I can follow-up with a fix/improvement for the debug info mangling issue we've been discussing here.

[rjmccall]

Thanks, Akira.

[ahatanak]

Here is the patch that removes the fake FunctionDecls: https://reviews.llvm.org/D104082

[ahatanak]

There are several files other than CGBlocks.cpp in which FunctionDecl::Create is called to create fake FunctionDecls. Do those places have to be fixed too?

[dblaikie]

In D98799#2820198, @ahatanak wrote:

There are several files other than CGBlocks.cpp in which FunctionDecl::Create is called to create fake FunctionDecls. Do those places have to be fixed too?

This issue in this particular case was that the function that was created never had its parameter info properly initialized (so that this call succeeds: https://github.com/llvm-mirror/clang/blob/aa231e4be75ac4759c236b755c57876f76e3cf05/lib/AST/ItaniumMangle.cpp#L2908 ). Any examples we could sample/look at?

[ahatanak]

I think these places have to be fixed too:
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/CGBuiltin.cpp#L1705
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/CGNonTrivialStruct.cpp#L476
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/CGObjC.cpp#L3692
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/CGObjC.cpp#L3776
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/ItaniumCXXABI.cpp#L2618

This one is okay since it's creating a function that doesn't take any parameters:
https://github.com/llvm/llvm-project/blob/bf9f21a28be171dc500cc68b4cb1fcd3fc33f229/clang/lib/CodeGen/CGStmtOpenMP.cpp#L449

[ahatanak]

I think I've fixed all the places in CodeGen that create fake FunctionDecls and would cause clang to crash.

[dblaikie]

In D98799#2850682, @ahatanak wrote:

I think I've fixed all the places in CodeGen that create fake FunctionDecls and would cause clang to crash.

Thanks, I really appreciate it! I'll have a go at unifying this mangled V unique internal linkage name stuff soon.

[dblaikie]

In D98799#2850700, @dblaikie wrote:

In D98799#2850682, @ahatanak wrote:

I think I've fixed all the places in CodeGen that create fake FunctionDecls and would cause clang to crash.

Thanks, I really appreciate it! I'll have a go at unifying this mangled V unique internal linkage name stuff soon.

Committed a patch to resolve the inconsistencies with K&R declarations, __attribute__((overloadable)) (which would mangle overloadable K&R declarations, but would not attach the mangled name to the debug info metadata), and -funique-internal-linkage-names (which wouldn't mangle/suffix K&R declarations (but would make this choice consistently between debug info and the actual symbol name) - now all those cases can/do mangle the names: 6c9559b67b91966bfeff9e17808a3e84a92e64a0

[hoy]

In D98799#2860881, @dblaikie wrote:

In D98799#2850700, @dblaikie wrote:

In D98799#2850682, @ahatanak wrote:

I think I've fixed all the places in CodeGen that create fake FunctionDecls and would cause clang to crash.

Thanks, I really appreciate it! I'll have a go at unifying this mangled V unique internal linkage name stuff soon.

Committed a patch to resolve the inconsistencies with K&R declarations, __attribute__((overloadable)) (which would mangle overloadable K&R declarations, but would not attach the mangled name to the debug info metadata), and -funique-internal-linkage-names (which wouldn't mangle/suffix K&R declarations (but would make this choice consistently between debug info and the actual symbol name) - now all those cases can/do mangle the names: 6c9559b67b91966bfeff9e17808a3e84a92e64a0

Thanks for the fix!

clang/lib/AST/ItaniumMangle.cpp

if (!NeedsUniqueInternalLinkageNames || !ND)
     return false;
 
   const auto *FD = dyn_cast<FunctionDecl>(ND);
   if (!FD)
     return false;
 
   // For C functions without prototypes, return false as their
   // names should not be mangled.
-  if (!FD->getType()->getAs<FunctionProtoType>())
+  if (!FD->hasPrototype())
     return false;
 
   if (isInternalLinkageDecl(ND))
     return true;
 
   return false;
 }
 

clang/lib/CodeGen/CGDebugInfo.cpp

 }
 
 void CGDebugInfo::collectFunctionDeclProps(GlobalDecl GD, llvm::DIFile *Unit,
                                            StringRef &Name,
                                            StringRef &LinkageName,
                                            llvm::DIScope *&FDContext,
                                            llvm::DINodeArray &TParamsArray,
                                            llvm::DINode::DIFlags &Flags) {
-  const auto *FD = cast<FunctionDecl>(GD.getDecl());
+  const auto *FD = cast<FunctionDecl>(GD.getCanonicalDecl().getDecl());
   Name = getFunctionName(FD);
   // Use mangled name as linkage name for C/C++ functions.
   if (FD->hasPrototype()) {
     LinkageName = CGM.getMangledName(GD);
     Flags |= llvm::DINode::FlagPrototyped;
   }
   // No need to replicate the linkage name if it isn't different from the
   // subprogram name, no need to have it at all unless coverage is enabled or
   // debug is set to more than just line tables or extra debug info is needed.

clang/test/CodeGen/unique-internal-linkage-names-dwarf.c

 // This test checks if C functions with internal linkage names are mangled
 // and the module hash suffixes attached including emitting DW_AT_linkage_name.
 //
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=4 -emit-llvm -o -  %s | FileCheck %s --check-prefix=PLAIN
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=4 -funique-internal-linkage-names -emit-llvm -o -  %s | FileCheck %s --check-prefix=UNIQUE
 //
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=5 -emit-llvm -o -  %s | FileCheck %s --check-prefix=PLAIN
 // RUN: %clang_cc1 -triple x86_64-unknown-linux -debug-info-kind=limited -dwarf-version=5 -funique-internal-linkage-names -emit-llvm -o -  %s | FileCheck %s --check-prefix=UNIQUE
 
 static int glob;
+// foo should be given a uniquefied name under -funique-internal-linkage-names.
 static int foo(void) {
   return glob;
 }
 
+// bar should not be given a uniquefied name under -funique-internal-linkage-names, 
+// since it doesn't come with valid prototype.
+static int bar(a) int a;
+{
+  return glob + a;
+}
+
+// go should be given a uniquefied name under -funique-internal-linkage-names, even 
+// if its definition doesn't come with a valid prototype, but the declaration here
+// has a prototype.
+static int go(int);
+
 void baz() {
   foo();
+  bar(1);
+  go(2);
 }
 
+static int go(a) int a;
+{
+  return glob + a;
+}
+
+
 // PLAIN: @glob = internal global i32
 // PLAIN: define internal i32 @foo()
+// PLAIN: define internal i32 @bar(i32 %a)
 // PLAIN: distinct !DIGlobalVariable(name: "glob"{{.*}})
 // PLAIN: distinct !DISubprogram(name: "foo"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "bar"{{.*}})
+// PLAIN: distinct !DISubprogram(name: "go"{{.*}})
 // PLAIN-NOT: linkageName:
 //
 // UNIQUE: @glob = internal global i32
 // UNIQUE: define internal i32 @_ZL3foov.[[MODHASH:__uniq.[0-9]+]]()
+// UNIQUE: define internal i32 @bar(i32 %a)
+// UNIQUE: define internal i32 @_ZL2goi.[[MODHASH]](i32 %a)
 // UNIQUE: distinct !DIGlobalVariable(name: "glob"{{.*}})
 // UNIQUE: distinct !DISubprogram(name: "foo", linkageName: "_ZL3foov.[[MODHASH]]"{{.*}})
+// UNIQUE: distinct !DISubprogram(name: "go", linkageName: "_ZL2goi.[[MODHASH]]"{{.*}})