Unique Names for Functions with Internal Linkage

[tmsriram]

Summary

This is a standalone patch and this would help Propeller do a better job of code layout as it can accurately attribute the profiles to the right internal linkage function.

This also helps SampledFDO/AutoFDO correctly associate sampled profiles to the right internal function. Currently, if there is more than one internal symbol foo, their profiles are aggregated by SampledFDO.

This patch adds a new clang option, -funique-internal-funcnames, to generate unique names for functions with internal linkage. This patch appends the md5 hash of the module name to the function symbol as a best effort to generate a unique name for symbols with internal linkage.

[MaskRay]

clang/lib/CodeGen/CodeGenModule.cpp

1111

std::string llvm::getUniqueModuleId(Module *M) {

What is the second parameter?

What is the rationale that InternalLinkage is picked here?

clang/lib/Frontend/CompilerInvocation.cpp

966

Just Args.hasArg(OPT_funique_internal_funcnames).

-fno-* is handled in clangDriver. CC1 does not need to know -fno-* if it defaults to false.

[tmsriram]

clang/lib/CodeGen/CodeGenModule.cpp

1111

This change to LLVM, https://reviews.llvm.org/D73310, contains the modifications to getUniqueModuleId to use the Module Identifier also in the hash. We found that when "-z muldefs" are used or when there are no globals defined in a module, a unique hash could not be generated. Using the full module name allows for a unique identifier in these cases. The second parameter basically allows use of module name and we did not force this to keep the existing usage as it is.

Internal Linkage is picked because there could be multiple functions of the same name with internal linkage in the final binary. So, associating sampled profile symbols to the right function name becomes a problem. Further, if multiple functions of the same name are hot, their profiles are aggregated affecting the optimization itself. So, a unique name for internal linkage functions will clearly disambiguate the profile information.

[tmsriram]

clang/lib/Frontend/CompilerInvocation.cpp

966

Are you suggesting I remove the -fno-* flag? The -fno-* is useful if -funique-internal-funcnames came added to a Makefile ,for example, and this needs to be disabled. This is similar to say -ffunction-sections. Please clarify. Thanks.

[tmsriram]

Use Hash of Module's source file name directly.

[tmsriram]

Remove an unnecessary header file inclusion.

[pcc]

clang/lib/CodeGen/CodeGenModule.cpp

1118

Why ".$" and not just "."?

[tmsriram]

clang/lib/CodeGen/CodeGenModule.cpp

1118

I just wanted to keep it consistent with what getUniqueModuleId does with the Md5 hash. I can remove it.

[MaskRay]

clang/include/clang/Driver/Options.td

1959

, Flags<[CC1Option]> can be deleted.

clang/lib/CodeGen/CodeGenModule.cpp

1110

Is this-> a must?

1118

Agreed. "." seems sufficient.

1119

MangledName += UniqueSuffix;

clang/lib/Frontend/CompilerInvocation.cpp

966

This file is about the cc1 option, which is not expected to be used by a user.

The driver has handled -fno-, so we don't have to repeat it here. -fmerge-functions and -fno-jump-tables are good examples that we don't have to add both -ffoo and -fno-foo to cc1 options.

[tmsriram]

Minor changes. Remove CC1option on "-fno", Remove "$" from unique suffix.

[MaskRay]

The code change seems fine, but the test requires some changes. I haven't followed Propeller development, but hope someone with profile experience can confirm InternalLinkage is the only linkage we need to care about (otherwise the option will be a misnomer if we ever extend it) and check whether this feature is useful on its own. Does it improve profile precision?

clang/test/CodeGen/unique_internal_funcnames.c

2

The convention is file-name.c , rather than file_name.c . The latter exists, but is a minority.

3

There should be two tests:

• one in test/Driver/funique-internal-funcnames.c, for -funique-internal-funcnames -fno-unique-internal-funcnames testing
• the other one should stay here (test/CodeGen), but should be a -S -emit-llvm test for IR output.

We don't normally do driver --> assembly testing.

4

-target x86_64 should just work (generic ELF). This feature is not tied to linux-gnu.

[mtrofin]

In D73307#1839829, @MaskRay wrote:

The code change seems fine, but the test requires some changes. I haven't followed Propeller development, but hope someone with profile experience can confirm InternalLinkage is the only linkage we need to care about (otherwise the option will be a misnomer if we ever extend it) and check whether this feature is useful on its own. Does it improve profile precision?

I can comment on the usefulness aspect: we had an earlier prototype of this, which we tried on a real-world application benchmark. The binary had ~10% of local statics exhibiting duplicate names. Ensuring unique names led to observable differences in the AFDO file (i.e. some of those functions had profiles that, before, were lost for one of the duplicates, and now were correctly attributed to the different functions), and a measurable performance improvement.

[mtrofin]

clang/lib/CodeGen/CodeGenModule.cpp

1112

Just a thought: md5 is a non-bijective transformation, afaik. How about using base64 encoding, with the caveat that we replace + with $ and / with _ (so it results in a valid name), and discard padding =

The value being, one can copy/paste that identifier, do the trivial conversion back to base64 ($->+, _->/) and get the module name. Useful when debugging, for example.

[MaskRay]

In D73307#1839880, @mtrofin wrote:

In D73307#1839829, @MaskRay wrote:

The code change seems fine, but the test requires some changes. I haven't followed Propeller development, but hope someone with profile experience can confirm InternalLinkage is the only linkage we need to care about (otherwise the option will be a misnomer if we ever extend it) and check whether this feature is useful on its own. Does it improve profile precision?

I can comment on the usefulness aspect: we had an earlier prototype of this, which we tried on a real-world application benchmark. The binary had ~10% of local statics exhibiting duplicate names. Ensuring unique names led to observable differences in the AFDO file (i.e. some of those functions had profiles that, before, were lost for one of the duplicates, and now were correctly attributed to the different functions), and a measurable performance improvement.

Thanks! Do you happen to have numbers about the code size increase? Every InternalLinkage function will have a longer time. They may take a significant portion of the string table (.strtab) size. If you strip .strtab, the profiling precision will be lowered to the situation before.

[mtrofin]

In D73307#1839984, @MaskRay wrote:

In D73307#1839880, @mtrofin wrote:

In D73307#1839829, @MaskRay wrote:

The code change seems fine, but the test requires some changes. I haven't followed Propeller development, but hope someone with profile experience can confirm InternalLinkage is the only linkage we need to care about (otherwise the option will be a misnomer if we ever extend it) and check whether this feature is useful on its own. Does it improve profile precision?

I can comment on the usefulness aspect: we had an earlier prototype of this, which we tried on a real-world application benchmark. The binary had ~10% of local statics exhibiting duplicate names. Ensuring unique names led to observable differences in the AFDO file (i.e. some of those functions had profiles that, before, were lost for one of the duplicates, and now were correctly attributed to the different functions), and a measurable performance improvement.

Thanks! Do you happen to have numbers about the code size increase? Every InternalLinkage function will have a longer time. They may take a significant portion of the string table (.strtab) size. If you strip .strtab, the profiling precision will be lowered to the situation before.

I assume you mean binary size increase. It was 0.8% larger in my case.

[tmsriram]

Address Reviewer comments:

• Add new driver test.
• Fix existing test to only check for code output.

clang/lib/CodeGen/CodeGenModule.cpp

1112

I dont think getting the module name back from the hash is super useful given that we can always inspect this symbol in gdb for source info.

[MaskRay]

clang/lib/Frontend/CompilerInvocation.cpp

960

Delete excess empty line.

clang/test/Driver/funique-internal-funcnames.c

3 ↗

(On Diff #251220)

CHECK-OPT: "-funique-internal-funcnames"

[tmsriram]

Address reviewer comments. Fix test and delete blank line.

[MaskRay]

clang/test/CodeGen/unique-internal-funcnames.c

3 ↗

(On Diff #251409)

You can hardly find any .c -> .s test in clang/test. We mostly do .c -> .ll testing. .ll -> .s are in llvm/test/CodeGen.

[lebedev.ri]

clang/lib/CodeGen/CodeGenModule.cpp

1107

maybe
identifier as this is a best-effort solution

clang/test/CodeGen/unique-internal-funcnames.c

16 ↗

(On Diff #251409)

What does getModule().getSourceFileName() contain?
The full path to the source file, or just the source file basename?
If latter, maybe check the full name?

[davidxl]

As far as I understand, this is not a propeller specific patch, but it fixes a general problem in AFDO as well. Perhaps change the summary to reflect that?

[davidxl]

clang/lib/CodeGen/CodeGenModule.cpp

1107

Address review comment here.

1113

Source filenames are not guaranteed to be unique, or it does contain the path as well?

clang/test/CodeGen/unique-internal-funcnames.c

3 ↗

(On Diff #251409)

Is this convention documented somewhere? Any concerns of producing .s?

[tmsriram]

clang/test/CodeGen/unique-internal-funcnames.c

3 ↗

(On Diff #251409)

There are more than 200 tests in clang that do -S from .c -> .s and tens of tests in CodeGen alone. I can change this to .ll but curious where this "hardly any" comes from?

[tmsriram]

Address reviewer comments.

• reword comment
• rewrite test to use -emit-llvm

[tmsriram]

clang/test/CodeGen/unique-internal-funcnames.c

16 ↗

(On Diff #251409)

It contains the full path.

[rnk]

I guess my initial reaction is that it is disappointing that downstream consumers can't cope with non-unique symbol names (What is the point of having internal linkage if we can't rely on it?), but given @mtrofin's input about AFDO, this seems like it may be a practical solution. I can think of at least two other cases where I have wanted similar functionality, the first being unique names for CodeView types in anonymous namespaces, the second being Jumbo support, producing one object from two .cpp source inputs.

To handle the case for type identifiers, we came up with this solution:
https://github.com/llvm/llvm-project/blob/master/clang/lib/AST/MicrosoftMangle.cpp#L401
Your solution is pretty similar.

In D73307#1839829, @MaskRay wrote:

I haven't followed Propeller development, but hope someone with profile experience can confirm InternalLinkage is the only linkage we need to care about (otherwise the option will be a misnomer if we ever extend it) and check whether this feature is useful on its own.

There is one other "local" linkage type, private linkage, which I believe corresponds with temporary assembler labels (.L*) that don't survive into the object file symbol table. I think the main use case for private globals is to save on symbol table size, so if the user passes this flag, it seems reasonable to upgrade the linkage to internal, and assign a unique name.

There is also the case of unnamed globals, which I think are private. I don't think clang ever generates these, so they are not very interesting.

I grepped the clang/test subdirectory for 'define.*private', and it seems there are no tests for private functions, so I doubt clang ever emits them.

---

At a higher level, should this just be an IR pass that clang adds into the pipeline when the flag is set? It should be safe to rename internal functions and give private functions internal linkage. It would be less invasive to clang and have better separation of concerns. As written, this is based on the source filename on the module, which is accessible from IR. The only reason I can think of against this is that the debug info might refer to the function linkage name, but maybe that is calculated later during codegen.

clang/lib/CodeGen/CodeGenModule.cpp

1039

We seem to be defeating NRVO with our "clever" small string type. :(

1103

There are several other callers of getMangledNameImpl. Should they also use this suffix? They mostly have to do with function multiversioning. Those seem like they could be internal and need this treatment.

1109

isa would be more idiomatic than testing dyn_cast results for truth.

1113

It appears to contain the path as the compiler receives it on the command line. However, it is possible to design a build where this string is not unique:

cd foo
clang -c name_conflict.c 
cd ../bar
clang -c name_conflict.c
clang foo/name_conflict.o bar/name_conflict.o

I don't think we should try to handle this case, but we should document the limitation somewhere. Some build systems do operate changing the working directory as they go.

Can you add some to the clang/docs/UsersManual.rst file? I'd expect it to show up here:
https://clang.llvm.org/docs/UsersManual.html#command-line-options

You can elaborate that the unique names are calculated using the source path passed to the compiler, and in a build system where that is not unique, the function names may not be unique.

---

I have also used this construct in the past for building single-file ABI compatibility test cases:

// foo.cpp
#ifdef PART1
// ...
#elif defined(PART2)
// ...
#endif

$ cc -c foo.cpp -DPART1
$ cc -c foo.cpp -DPART2

1117

I think it would be preferable to calculate the source filename MD5 hash once, when CodeGenModule is constructed, stringify it, and then use that every time we mangle a name.

clang/test/CodeGen/unique-internal-funcnames.c

3 ↗

(On Diff #251409)

I can't specifically find documentation for this convention, but it is generally understood that every feature should have targeted unit tests that exercise the minimum amount of functionality. In this case, that means using %clang_cc1 -emit-llvm -triple NNN and checking the IR.

[davidxl]

clang/lib/CodeGen/CodeGenModule.cpp

1113

yes, the first example was the scenario I meant. I agree name conflict due that case should be really rare. If yes, we can always use content based uniqueid -- by appending llvm::getUniqueModuleId -- but that is probably overkill.

[hubert.reinterpretcast]

clang/lib/CodeGen/CodeGenModule.cpp

1113

yes, the first example was the scenario I meant. I agree name conflict due that case should be really rare.

Except for projects where it is the rule and not the exception. One pattern where this occurs is when each subdirectory implements one class and there are multiple classes that implement the same interface. In each directory, the implementation of each class is separated into files by grouping methods of the class. The set of filenames in one such directory may well be the same as the set in another.

[MaskRay]

clang/test/CodeGen/unique-internal-funcnames.c

3 ↗

(On Diff #251409)

Not that many -S .c -> .s tests (rg '%clang.* -S ' | egrep -v '[-]###|emit-llvm|-verify' | awk -F: '{print $1}' | sort -u | wc -l). Many are about -emit-llvm -### -verify or misused -v. They do not need to produce assembly.

clang/test/Driver/funique-internal-funcnames.c

4 ↗

(On Diff #251469)

This negative pattern needs to be changed as well.

[tmsriram]

clang/lib/CodeGen/CodeGenModule.cpp

1113

I am not sure much can be done here other than try using getUniqueModuleId. Just to be clear, even if the file names are the same, the actual conflict happens only when the "make " is done from the leaf directory. Doing:

cc a/name_conflict.cpp -c
cc b/name_conflict.cpp -c

should still produce unique names.

[hubert.reinterpretcast]

clang/lib/CodeGen/CodeGenModule.cpp

1113

Understood. I do happen to have been working on a project that does the whole $(MAKE) -C a/ thing with such a project layout. Content based hashing is unfortunate for other reasons: The hash would be unstable during the course of maintenance on the codebase. I guess the limitation is okay for the intended use case of the patch.

[tmsriram]

Address @rnk comments:

• Moved implementation to getMangledNameImpl to catch multi-versioned symbols and internal linkage globals
• Moved hash computation to consrtuctor
• Renamed option to -funique-internal-linkage-names
• Add documentation to UsersManual.rst and verified with html conversion
• Added tests for MV symbols and static globals, is there anything else? I am not too familiar with internal/private symbol possibilites

[tmsriram]

clang/lib/CodeGen/CodeGenModule.cpp

1103

I moved the suffix append to getMangledNameImpl. Other mangled name manipulations are also done here. Added tests for multiversioned internal linkage symbols.

1113

Acknowledged.

[tmsriram]

Ping.

[davidxl]

clang/docs/UsersManual.rst

1683 ↗

(On Diff #251868)

Is it necessary to document 'MD5 hash' as it is an implementation detail? Perhaps just 'the name hash'

1689 ↗

(On Diff #251868)

if the two modules contain symbols with the same private linkage name.

clang/lib/CodeGen/CodeGenModule.cpp

1026

is the first check redundant now?

[tmsriram]

Changes to description of flag, remove redundant check.

[MaskRay]

In D73307#1932131, @rnk wrote:

At a higher level, should this just be an IR pass that clang adds into the pipeline when the flag is set? It should be safe to rename internal functions and give private functions internal linkage. It would be less invasive to clang and have better separation of concerns. As written, this is based on the source filename on the module, which is accessible from IR. The only reason I can think of against this is that the debug info might refer to the function linkage name, but maybe that is calculated later during codegen.

I second this suggestion. clang/lib/CodeGen/BackendUtil.cpp EmitAssemblyHelper::EmitAssemblyWithNewPassManager ModulePassManager may be a more appropriate place for this feature. There are examples from both sides:

• clang frontend: #pragma redefine_extname
• middle end IR->IR: the old pass manager has a feature -frewrite-map-file, which does a similar thing but is implemented as an IR transformation.

[tmsriram]

In D73307#1942805, @MaskRay wrote:

In D73307#1932131, @rnk wrote:

At a higher level, should this just be an IR pass that clang adds into the pipeline when the flag is set? It should be safe to rename internal functions and give private functions internal linkage. It would be less invasive to clang and have better separation of concerns. As written, this is based on the source filename on the module, which is accessible from IR. The only reason I can think of against this is that the debug info might refer to the function linkage name, but maybe that is calculated later during codegen.

I second this suggestion. clang/lib/CodeGen/BackendUtil.cpp EmitAssemblyHelper::EmitAssemblyWithNewPassManager ModulePassManager may be a more appropriate place for this feature. There are examples from both sides:

@rnk getMangledNameImpl for example adds multi-versioning suffixes, "regcall3_" and "device_stub". Adding the hash is just so simple. Is a pass really needed? If so, I should parse ".<target>" suffixed Multi-versioining names and insert the hash in between as the demangling looks better?

• clang frontend: #pragma redefine_extname
• middle end IR->IR: the old pass manager has a feature -frewrite-map-file, which does a similar thing but is implemented as an IR transformation.

clang/lib/CodeGen/CodeGenModule.cpp

1026

Good catch, converted to an assert.

[MaskRay]

In D73307#1942838, @tmsriram wrote:

In D73307#1942805, @MaskRay wrote:

In D73307#1932131, @rnk wrote:

At a higher level, should this just be an IR pass that clang adds into the pipeline when the flag is set? It should be safe to rename internal functions and give private functions internal linkage. It would be less invasive to clang and have better separation of concerns. As written, this is based on the source filename on the module, which is accessible from IR. The only reason I can think of against this is that the debug info might refer to the function linkage name, but maybe that is calculated later during codegen.

I second this suggestion. clang/lib/CodeGen/BackendUtil.cpp EmitAssemblyHelper::EmitAssemblyWithNewPassManager ModulePassManager may be a more appropriate place for this feature. There are examples from both sides:

@rnk getMangledNameImpl for example adds multi-versioning suffixes, "regcall3_" and "device_stub". Adding the hash is just so simple. Is a pass really needed? If so, I should parse ".<target>" suffixed Multi-versioining names and insert the hash in between as the demangling looks better?

• clang frontend: #pragma redefine_extname
• middle end IR->IR: the old pass manager has a feature -frewrite-map-file, which does a similar thing but is implemented as an IR transformation.

There may be some other interactions @rnk noticed. I am happy with either way.

clang/test/CodeGen/unique-internal-linkage-names.cpp

5 ↗

(On Diff #252688)

Might be worth adding a few other internal linkage names.

• an object in an unnamed namespace
• an extern "C" static function
• a function-local static variable
• label: &&label

Hope @mtrofin and @davidxl can clarify what internal names may benefit AFDO and we can add such internal names specifically.

[MaskRay]

clang/lib/CodeGen/CodeGenModule.cpp

1103

Delete this unrelated cosmetic change.

[wmi]

clang/test/CodeGen/unique-internal-linkage-names.cpp

5 ↗

(On Diff #252688)

Only internal functions matter for AFDO. Other types of internal names are irrelevant.

[tmsriram]

clang/test/CodeGen/unique-internal-linkage-names.cpp

5 ↗

(On Diff #252688)

extern "C" static is not very useful as the name is not exposed outside the object file. C++ compiler will still mangle the name. for eg:

a.cpp

extern "C" {

static int bar() {
  return 0;
}

}

int foo() {

printf("%p\n", bar);

}

$ nm a.o
0000000000000040 t _ZL3barv

Also, for label: &&label, it is not preserved in the symbol table as a .L<name> is used which is deleted by the assembler. I can throw a asm("") directive but that will bypass the front-end. Is there anyway to preserve this?

I can add the rest of the cases.

[davidxl]

clang/test/CodeGen/unique-internal-linkage-names.cpp

29 ↗

(On Diff #252688)

[a side note]: interesting, since when Clang had this target attribute based multi-versioning and function overloading ? Sriraman added this support in GCC 8 years ago. Did not realize it is in Clang too :)

[tmsriram]

Rebase and add tests for anonymous namespace symbol and function static global.

[tmsriram]

Ping.

[rnk]

Sorry for the delay, thanks for the update. Code looks good, but I want the documentation to be specific.

clang/docs/UsersManual.rst

1684 ↗

(On Diff #254347)

I think it's important to be specific here. I'd go so far as to say:
"When this option is set, compiler hashes the main source file path from the command line and appends it to all internal symbols. If a program contains multiple objects compiled from the same source file path, the symbols are not guaranteed to be unique."

[MaskRay]

clang/docs/UsersManual.rst

1684 ↗

(On Diff #254347)

Should option like -ffile-prefix-map= affect the hashed source file path? I suspect it already does this but I haven't verified.

This may need tests for Windows and non-Windows, similar to D77223.

[hubert.reinterpretcast]

clang/docs/UsersManual.rst

1684 ↗

(On Diff #254347)

With respect to @rnk's suggestion, "from the same source file path" implies the same file. I would suggest saying "with the same command-line source file specification".

[tmsriram]

Change description and handle -ffile-prefix-map/-fmacro-prefix-map.

clang/docs/UsersManual.rst

1684 ↗

(On Diff #254347)

I have handled this now and added a test. I saw the reference but don't quite understand the need for windows test? I am not familiar with this.

[tmsriram]

We just noticed an issue with alias attribute and this option. Here is the code that exposes the problem:

alias_global.c

static int foo;
extern int bar attribute((alias("foo")))

$ clang -c alias_global.c -funique-internal-linkage-names
alias_global.c:4:31: error: alias must point to a defined variable or function
extern int bar attribute((alias("foo")));

This happens because bar is aliased to "foo" and not "foo.<md5sum>".

I would like to know your thoughts on this? Here is what I think. alias attribute is a bit special in that the symbol name must be given carefully, for instance the mangled name if it is a C++ compile and the actual name for a C compile. Even without the option, the same error will happen if a C++ compiler was used on this code. So, in some sense, alias attribute requires the user to know what symbol name to expect. -funique-internal-linkage-names can be treated as just an additional extension of this where the user must append the md5 hash of the module, which can be computed independently with:

$ echo -n alias_global.c | md5sum

So, I suggest we leave it as it is and add to the documentation that this needs to be kept in mind.

[rnk]

Regarding the alias attribute, it occurs to me that reimplementing this as an early LLVM pass would not have that problem. Do you think that would be worth doing?

clang/docs/UsersManual.rst

1684 ↗

(On Diff #254347)

Would the file prefix map not already have been applied when creating the LLVM IR module source file path? I think it should be, so I'm not sure the new code you added is in the right place.

clang/lib/CodeGen/CodeGenModule.h

31 ↗

(On Diff #255871)

Why add this include here? I don't see any reason it is needed. Maybe the cpp file needs it?

[tmsriram]

In D73307#1972388, @rnk wrote:

Regarding the alias attribute, it occurs to me that reimplementing this as an early LLVM pass would not have that problem. Do you think that would be worth doing?

I can try doing this. If my understanding is right, you are suggesting that doing this later would mean the alias would have already been applied?. However, for multi-versioning symbols, I must parse the target name to insert the module name in between right?

clang/docs/UsersManual.rst

1684 ↗

(On Diff #254347)

Module.getSourceFileName() never changes. I checked here and also at AsmPrinter.cpp. I am not sure this a bug, maybe I should just fix that first.

clang/lib/CodeGen/CodeGenModule.h

31 ↗

(On Diff #255871)

Yes, the cpp file needs this to access the MacroPrefixMap.

[tmsriram]

Updated this patch, using a pass to convert symbol names in D78243

Also, removed the test for -fmacro-prefix-map. For -ffile-prefix-map, looks like getSourceFileName() should be modified to contain the new path.

[rnk]

In D73307#1978140, @tmsriram wrote:

In D73307#1972388, @rnk wrote:

Regarding the alias attribute, it occurs to me that reimplementing this as an early LLVM pass would not have that problem. Do you think that would be worth doing?

I can try doing this. If my understanding is right, you are suggesting that doing this later would mean the alias would have already been applied?. However, for multi-versioning symbols, I must parse the target name to insert the module name in between right?

Yes, Clang will apply the alias, do all name mangling, etc, and then renaming will happen later.

It's not clear to me if the order of the .<target> suffix on multi-versioned symbols matters. @erichkeane, should it? Supposing some mid-level optimization came along and did function versioning, it would rename the internal function and append .1. So, I think appending as you implemented is fine.

---

All of my concerns have been addressed, and I think everyone else's are as well. We have documentation indicating what we mean by uniqueness which @hubert.reinterpretcast wanted clarified, we had the prefix map issue which I believe is addressed, so to my knowledge this is ready.

Let's wait a bit to hear from Erich, but otherwise I think this is ready in a few days.

[erichkeane]

In D73307#1989740, @rnk wrote:

In D73307#1978140, @tmsriram wrote:

In D73307#1972388, @rnk wrote:

Regarding the alias attribute, it occurs to me that reimplementing this as an early LLVM pass would not have that problem. Do you think that would be worth doing?

I can try doing this. If my understanding is right, you are suggesting that doing this later would mean the alias would have already been applied?. However, for multi-versioning symbols, I must parse the target name to insert the module name in between right?

Yes, Clang will apply the alias, do all name mangling, etc, and then renaming will happen later.

It's not clear to me if the order of the .<target> suffix on multi-versioned symbols matters. @erichkeane, should it? Supposing some mid-level optimization came along and did function versioning, it would rename the internal function and append .1. So, I think appending as you implemented is fine.

---

All of my concerns have been addressed, and I think everyone else's are as well. We have documentation indicating what we mean by uniqueness which @hubert.reinterpretcast wanted clarified, we had the prefix map issue which I believe is addressed, so to my knowledge this is ready.

Let's wait a bit to hear from Erich, but otherwise I think this is ready in a few days.

Multiversioning doesn't care about the names, simply that they are unique/unique from eachother. The suffix that GCC/ICC put together for these two types is simply to make sure we don't get something that conflicts with another symbol. As long as these symbols are unique (and correclty updated!) , they can be named absolutely anything.

[tmsriram]

In D73307#1989924, @erichkeane wrote:

In D73307#1989740, @rnk wrote:

In D73307#1978140, @tmsriram wrote:

In D73307#1972388, @rnk wrote:

Regarding the alias attribute, it occurs to me that reimplementing this as an early LLVM pass would not have that problem. Do you think that would be worth doing?

I can try doing this. If my understanding is right, you are suggesting that doing this later would mean the alias would have already been applied?. However, for multi-versioning symbols, I must parse the target name to insert the module name in between right?

Yes, Clang will apply the alias, do all name mangling, etc, and then renaming will happen later.

It's not clear to me if the order of the .<target> suffix on multi-versioned symbols matters. @erichkeane, should it? Supposing some mid-level optimization came along and did function versioning, it would rename the internal function and append .1. So, I think appending as you implemented is fine.

---

All of my concerns have been addressed, and I think everyone else's are as well. We have documentation indicating what we mean by uniqueness which @hubert.reinterpretcast wanted clarified, we had the prefix map issue which I believe is addressed, so to my knowledge this is ready.

Let's wait a bit to hear from Erich, but otherwise I think this is ready in a few days.

Multiversioning doesn't care about the names, simply that they are unique/unique from eachother. The suffix that GCC/ICC put together for these two types is simply to make sure we don't get something that conflicts with another symbol. As long as these symbols are unique (and correclty updated!) , they can be named absolutely anything.

In D73307#1989924, @erichkeane wrote:

In D73307#1989740, @rnk wrote:

In D73307#1978140, @tmsriram wrote:

In D73307#1972388, @rnk wrote:

Regarding the alias attribute, it occurs to me that reimplementing this as an early LLVM pass would not have that problem. Do you think that would be worth doing?

I can try doing this. If my understanding is right, you are suggesting that doing this later would mean the alias would have already been applied?. However, for multi-versioning symbols, I must parse the target name to insert the module name in between right?

Yes, Clang will apply the alias, do all name mangling, etc, and then renaming will happen later.

It's not clear to me if the order of the .<target> suffix on multi-versioned symbols matters. @erichkeane, should it? Supposing some mid-level optimization came along and did function versioning, it would rename the internal function and append .1. So, I think appending as you implemented is fine.

---

All of my concerns have been addressed, and I think everyone else's are as well. We have documentation indicating what we mean by uniqueness which @hubert.reinterpretcast wanted clarified, we had the prefix map issue which I believe is addressed, so to my knowledge this is ready.

Let's wait a bit to hear from Erich, but otherwise I think this is ready in a few days.

Multiversioning doesn't care about the names, simply that they are unique/unique from eachother. The suffix that GCC/ICC put together for these two types is simply to make sure we don't get something that conflicts with another symbol. As long as these symbols are unique (and correclty updated!) , they can be named absolutely anything.

Thanks for the comments! I was one of the authors of the GCC multiversioning patch, and if I recall correctly from years ago, one of the reasons for naming the target clones of multi-versioned foo as foo, foo.sse, etc. was so that c++filt works nicely on this. Like, c++filt on _Z3foov.see would demangle as foo() [clone sse]. But c++filt is broken IMO as it is not handling these suffixes correctly any more, like c++filt _Z3foov.sse4.2 does not even demangle as expected.

[erichkeane]

Thanks for the comments! I was one of the authors of the GCC multiversioning patch, and if I recall correctly from years ago, one of the reasons for naming the target clones of multi-versioned foo as foo, foo.sse, etc. was so that c++filt works nicely on this. Like, c++filt on _Z3foov.see would demangle as foo() [clone sse]. But c++filt is broken IMO as it is not handling these suffixes correctly any more, like c++filt _Z3foov.sse4.2 does not even demangle as expected.

Ah :) I implemented it here, it would have been great to have picked your brain on it back when implementing MV!

c++filt seems hit or miss as to whether it would demangle it. i think ones with dots in it (sse4.2) doesn't work.

I also implemented CPU-Dispatch/CPU-Specific from ICC at one point which doesn't demangle at all.

It DOES remind me, I had target-clones implemented at one point and under review, but never got back around to it...

[tmsriram]

@rnk Good now?

[tmsriram]

Ping.

[tmsriram]

@rnk Ping.

[rnk]

lgtm

Sorry for the delay.

[tmsriram]

Rebase.

[tmsriram]

Update patch with changes to BackendUtil.cpp (forgot this file).

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(NoSignedZeros     , 1, 0) ///< Allow ignoring the signedness of FP zero
 CODEGENOPT(NullPointerIsValid , 1, 0) ///< Assume Null pointer deference is defined.
 CODEGENOPT(Reassociate       , 1, 0) ///< Allow reassociation of FP math ops
 CODEGENOPT(ReciprocalMath    , 1, 0) ///< Allow FP divisions to be reassociated.
 CODEGENOPT(NoTrappingMath    , 1, 0) ///< Set when -fno-trapping-math is enabled.
 CODEGENOPT(NoNaNsFPMath      , 1, 0) ///< Assume FP arguments, results not NaN.
 CODEGENOPT(FlushDenorm       , 1, 0) ///< Allow FP denorm numbers to be flushed to zero
 CODEGENOPT(CorrectlyRoundedDivSqrt, 1, 0) ///< -cl-fp32-correctly-rounded-divide-sqrt
+CODEGENOPT(UniqueInternalFuncNames, 1, 0) ///< Internal Linkage functions get unique names.
 
 /// When false, this attempts to generate code as if the result of an
 /// overflowing conversion matches the overflowing behavior of a target's native
 /// float-to-int conversion instructions.
 CODEGENOPT(StrictFloatCastOverflow, 1, 1)
 
 CODEGENOPT(UniformWGSize     , 1, 0) ///< -cl-uniform-work-group-size
 CODEGENOPT(NoZeroInitializedInBSS , 1, 0) ///< -fno-zero-initialized-in-bss.

clang/include/clang/Driver/Options.td

def fno_stack_size_section : Flag<["-"], "fno-stack-size-section">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Don't emit section containing metadata on function stack sizes">;
 
 def funique_section_names : Flag <["-"], "funique-section-names">,
   Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Use unique names for text and data sections (ELF Only)">;
 def fno_unique_section_names : Flag <["-"], "fno-unique-section-names">,
   Group<f_Group>, Flags<[CC1Option]>;
 
+def funique_internal_funcnames : Flag <["-"], "funique-internal-funcnames">,
+  Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Uniqueify Internal Linkage Function Names">;
+def fno_unique_internal_funcnames : Flag <["-"], "fno-unique-internal-funcnames">,
+  Group<f_Group>, Flags<[CC1Option]>;
+
 def fstrict_return : Flag<["-"], "fstrict-return">, Group<f_Group>,
   Flags<[CC1Option]>,
   HelpText<"Always treat control flow paths that fall off the end of a "
            "non-void function as unreachable">;
 def fno_strict_return : Flag<["-"], "fno-strict-return">, Group<f_Group>,
   Flags<[CC1Option]>;
 
 def fallow_editor_placeholders : Flag<["-"], "fallow-editor-placeholders">,

clang/lib/CodeGen/CodeGenModule.cpp

 #include "llvm/IR/ProfileSummary.h"
 #include "llvm/ProfileData/InstrProfReader.h"
 #include "llvm/Support/CodeGen.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ConvertUTF.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Support/TimeProfiler.h"
+#include "llvm/Transforms/Utils/ModuleUtils.h"
 
 using namespace clang;
 using namespace CodeGen;
 
 static llvm::cl::opt<bool> LimitedCoverage(
     "limited-coverage-experimental", llvm::cl::ZeroOrMore, llvm::cl::Hidden,
     llvm::cl::desc("Emit limited coverage mapping information (experimental)"),
     llvm::cl::init(false));
if (MC.shouldMangleDeclName(ND)) {
     } else {
       Out << II->getName();
     }
   }
 
   if (const auto *FD = dyn_cast<FunctionDecl>(ND))
     if (FD->isMultiVersion() && !OmitMultiVersionMangling) {
       switch (FD->getMultiVersionKind()) {
       case MultiVersionKind::CPUDispatch:
       case MultiVersionKind::CPUSpecific:
         AppendCPUSpecificCPUDispatchMangling(CGM,
                                              FD->getAttr<CPUSpecificAttr>(),
                                              GD.getMultiVersionIndex(), Out);
         break;
       case MultiVersionKind::Target:
         AppendTargetMangling(CGM, FD->getAttr<TargetAttr>(), Out);
         break;
       case MultiVersionKind::None:
         llvm_unreachable("None multiversion type isn't valid here");
       }
     }
 
   return Out.str();
 }
 
 void CodeGenModule::UpdateMultiVersionNames(GlobalDecl GD,
                                             const FunctionDecl *FD) {
   if (!FD->isMultiVersion())
     return;
 
   // Get the name of what this would be without the 'target' attribute.  This
StringRef CodeGenModule::getMangledName(GlobalDecl GD) {
   }
 
   auto FoundName = MangledDeclNames.find(CanonicalGD);
   if (FoundName != MangledDeclNames.end())
     return FoundName->second;
 
   // Keep the first result in the case of a mangling collision.
   const auto *ND = cast<NamedDecl>(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
+  // With option -funique-internal-funcnames, functions with internal linkage
+  // should get unique names.  Use "getUniqueModuleId" to get a unique
+  // identifier and this is a best effort.
+  if (getCodeGenOpts().UniqueInternalFuncNames &&
+      dyn_cast<FunctionDecl>(GD.getDecl()) &&
+      this->getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage &&
+      !getModule().getSourceFileName().empty()) {
+    llvm::MD5 Md5;
+    Md5.update(getModule().getSourceFileName());
+    llvm::MD5::MD5Result R;
+    Md5.final(R);
+    SmallString<32> Str;
+    llvm::MD5::stringifyResult(R, Str);
+    std::string UniqueSuffix = (".$" + Str).str();
+    MangledName = MangledName + UniqueSuffix;
+  }
+
   // Adjust kernel stub mangling as we may need to be able to differentiate
   // them from the kernel itself (e.g., for HIP).
   if (auto *FD = dyn_cast<FunctionDecl>(GD.getDecl()))
     if (!getLangOpts().CUDAIsDevice && FD->hasAttr<CUDAGlobalAttr>())
       MangledName = getCUDARuntime().getDeviceStubName(MangledName);
 
   auto Result = Manglings.insert(std::make_pair(MangledName, GD));
   return MangledDeclNames[CanonicalGD] = Result.first->first();

clang/lib/Driver/ToolChains/Clang.cpp

if (C.getDriver().embedBitcodeInObject() && !C.getDriver().isUsingLTO() &&
     // also reject kernel/kext
     static const constexpr unsigned kBitcodeOptionBlacklist[] = {
         options::OPT_mkernel,
         options::OPT_fapple_kext,
         options::OPT_ffunction_sections,
         options::OPT_fno_function_sections,
         options::OPT_fdata_sections,
         options::OPT_fno_data_sections,
+        options::OPT_funique_internal_funcnames,
+        options::OPT_fno_unique_internal_funcnames,
         options::OPT_funique_section_names,
         options::OPT_fno_unique_section_names,
         options::OPT_mrestrict_it,
         options::OPT_mno_restrict_it,
         options::OPT_mstackrealign,
         options::OPT_mno_stackrealign,
         options::OPT_mstack_alignment,
         options::OPT_mcmodel_EQ,
if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
                    UseSeparateSections)) {
     CmdArgs.push_back("-fdata-sections");
   }
 
   if (!Args.hasFlag(options::OPT_funique_section_names,
                     options::OPT_fno_unique_section_names, true))
     CmdArgs.push_back("-fno-unique-section-names");
 
+  if (Args.hasFlag(options::OPT_funique_internal_funcnames,
+                   options::OPT_fno_unique_internal_funcnames, false))
+    CmdArgs.push_back("-funique-internal-funcnames");
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
                   options::OPT_finstrument_functions_after_inlining,
                   options::OPT_finstrument_function_entry_bare);
 
   // NVPTX doesn't support PGO or coverage. There's no runtime support for
   // sampling, overhead of call arc collection is way too high and there's no
   // way to collect the output.
   if (!Triple.isNVPTX())

clang/lib/Frontend/CompilerInvocation.cpp

if (Opts.ThreadModel != "posix" && Opts.ThreadModel != "single")
     Diags.Report(diag::err_drv_invalid_value)
         << Args.getLastArg(OPT_mthread_model)->getAsString(Args)
         << Opts.ThreadModel;
   Opts.TrapFuncName = Args.getLastArgValue(OPT_ftrap_function_EQ);
   Opts.UseInitArray = !Args.hasArg(OPT_fno_use_init_array);
 
   Opts.FunctionSections = Args.hasFlag(OPT_ffunction_sections,
                                        OPT_fno_function_sections, false);
   Opts.DataSections = Args.hasFlag(OPT_fdata_sections,
                                    OPT_fno_data_sections, false);
   Opts.StackSizeSection =
       Args.hasFlag(OPT_fstack_size_section, OPT_fno_stack_size_section, false);
   Opts.UniqueSectionNames = Args.hasFlag(OPT_funique_section_names,
                                          OPT_fno_unique_section_names, true);
 
+  Opts.UniqueInternalFuncNames = Args.hasFlag(
+      OPT_funique_internal_funcnames, OPT_fno_unique_internal_funcnames, false);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
 
   Opts.NullPointerIsValid = Args.hasArg(OPT_fno_delete_null_pointer_checks);
 
   Opts.ProfileSampleAccurate = Args.hasArg(OPT_fprofile_sample_accurate);
 

clang/test/CodeGen/unique_internal_funcnames.c

+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -fno-unique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=PLAIN
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -S -funique-internal-funcnames -o - < %s | FileCheck %s --check-prefix=UNIQUE
+
+static int foo() {
+  return 0;
+}
+
+int (*bar())() {
+  return foo;
+}
+
+// PLAIN: foo:
+// UNIQUE-NOT: foo:
+// UNIQUE: foo.${{[0-9a-f]+}}: