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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
