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

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1125
   const auto *ND = cast<NamedDecl>(GD.getDecl());
   std::string MangledName = getMangledNameImpl(*this, GD, ND);
 
----------------
rnk wrote:
> 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.
I moved the suffix append to getMangledNameImpl.  Other mangled name manipulations are also done here.  Added tests for multiversioned internal linkage symbols.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1135
+    llvm::MD5 Md5;
+    Md5.update(getModule().getSourceFileName());
+    llvm::MD5::MD5Result R;
----------------
hubert.reinterpretcast wrote:
> tmsriram wrote:
> > hubert.reinterpretcast wrote:
> > > davidxl wrote:
> > > > rnk wrote:
> > > > > davidxl wrote:
> > > > > > Source filenames are not guaranteed to be unique, or it does contain the path as well?
> > > > > 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
> > > > > ```
> > > > 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.
> > > > 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.
> > 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.
> 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.
Acknowledged.


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

https://reviews.llvm.org/D73307