[PATCH] D73307: Unique Names for Functions with Internal Linkage

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked 2 inline comments as done.
tmsriram added a comment.

In D73307#1972388 <https://reviews.llvm.org/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?



================
Comment at: clang/docs/UsersManual.rst:1684
+   linkage symbols. The unique name is obtained by appending the hash of the
+   full module name to the original symbol. This option is particularly useful
+   in attributing profile information to the correct function when multiple
----------------
rnk wrote:
> tmsriram wrote:
> > hubert.reinterpretcast wrote:
> > > MaskRay wrote:
> > > > rnk wrote:
> > > > > 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."
> > > > 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.
> > > 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".
> > 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.
> 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.
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.


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:31
 #include "clang/Basic/XRayLists.h"
+#include "clang/Lex/PreprocessorOptions.h"
 #include "llvm/ADT/DenseMap.h"
----------------
rnk wrote:
> Why add this include here? I don't see any reason it is needed. Maybe the cpp file needs it?
Yes, the cpp file needs this to access the MacroPrefixMap.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73307/new/

https://reviews.llvm.org/D73307