Page MenuHomePhabricator

Unique Names for Functions with Internal Linkage
ClosedPublic

Authored by tmsriram on Jan 23 2020, 4:43 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
tmsriram marked 2 inline comments as done.Mar 19 2020, 10:13 AM

Address reviewer comments. Fix test and delete blank line.

MaskRay added inline comments.Mar 19 2020, 10:27 AM
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 added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
1152

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?

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 added inline comments.Mar 19 2020, 12:01 PM
clang/lib/CodeGen/CodeGenModule.cpp
1152

Address review comment here.

1158

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 marked an inline comment as done.Mar 19 2020, 12:48 PM
tmsriram added inline comments.
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 updated this revision to Diff 251469.Mar 19 2020, 1:55 PM
tmsriram marked 4 inline comments as done.

Address reviewer comments.

  • reword comment
  • rewrite test to use -emit-llvm
tmsriram edited the summary of this revision. (Show Details)Mar 19 2020, 1:57 PM
tmsriram added inline comments.
clang/test/CodeGen/unique-internal-funcnames.c
16 ↗(On Diff #251409)

It contains the full path.

rnk added a comment.Mar 19 2020, 1:57 PM

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.

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
1062

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

1148

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.

1154

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

1158

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
1162

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 added inline comments.Mar 19 2020, 2:47 PM
clang/lib/CodeGen/CodeGenModule.cpp
1158

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 added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
1158

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 added inline comments.Mar 19 2020, 3:23 PM
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 marked 4 inline comments as done.Mar 19 2020, 3:36 PM
tmsriram added inline comments.
clang/lib/CodeGen/CodeGenModule.cpp
1158

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.

clang/lib/CodeGen/CodeGenModule.cpp
1158

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 updated this revision to Diff 251868.Mar 21 2020, 5:16 PM
tmsriram marked 11 inline comments as done.

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 added inline comments.Mar 21 2020, 5:17 PM
clang/lib/CodeGen/CodeGenModule.cpp
1148

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

1158

Acknowledged.

davidxl added inline comments.Mar 25 2020, 11:01 AM
clang/docs/UsersManual.rst
1683

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

1689

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

clang/lib/CodeGen/CodeGenModule.cpp
1060

is the first check redundant now?

tmsriram updated this revision to Diff 252688.Mar 25 2020, 3:15 PM
tmsriram marked 4 inline comments as done.

Changes to description of flag, remove redundant check.

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.
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
1060

Good catch, converted to an assert.

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
6

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 added inline comments.Mar 30 2020, 4:12 PM
clang/lib/CodeGen/CodeGenModule.cpp
1148

Delete this unrelated cosmetic change.

wmi added inline comments.Mar 30 2020, 5:46 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
6

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

tmsriram marked an inline comment as done.Mar 30 2020, 6:08 PM
tmsriram added inline comments.
clang/test/CodeGen/unique-internal-linkage-names.cpp
6

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 added inline comments.Apr 1 2020, 3:49 PM
clang/test/CodeGen/unique-internal-linkage-names.cpp
30

[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 updated this revision to Diff 254347.Apr 1 2020, 4:47 PM

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

rnk added a comment.Apr 6 2020, 1:06 PM

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

clang/docs/UsersManual.rst
1684

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 added inline comments.Apr 6 2020, 1:47 PM
clang/docs/UsersManual.rst
1684

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.

clang/docs/UsersManual.rst
1684

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 updated this revision to Diff 255871.Apr 7 2020, 6:33 PM
tmsriram marked 4 inline comments as done.

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

clang/docs/UsersManual.rst
1684

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.

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 added a comment.Apr 9 2020, 11:01 AM

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

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

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

tmsriram marked 2 inline comments as done.Apr 13 2020, 10:11 AM
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

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

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

tmsriram updated this revision to Diff 257853.Apr 15 2020, 2:27 PM

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 added a subscriber: erichkeane.Apr 17 2020, 1:56 PM
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.

In D73307#1989740, @rnk 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#1989740, @rnk 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#1989740, @rnk 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.

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 marked 3 inline comments as done.Apr 21 2020, 10:43 AM

@rnk Good now?

rnk accepted this revision.May 5 2020, 4:45 PM

lgtm

Sorry for the delay.

This revision is now accepted and ready to land.May 5 2020, 4:45 PM
tmsriram updated this revision to Diff 262797.May 7 2020, 5:53 PM

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

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 7 2020, 6:28 PM