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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/include/clang/Driver/Options.td:1959
+def fno_unique_internal_funcnames : Flag <["-"], "fno-unique-internal-funcnames">,
+  Group<f_Group>, Flags<[CC1Option]>;
+
----------------
`, Flags<[CC1Option]>` can be deleted.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1109
+      dyn_cast<FunctionDecl>(GD.getDecl()) &&
+      this->getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage &&
+      !getModule().getSourceFileName().empty()) {
----------------
Is `this->` a must?


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1117
+    llvm::MD5::stringifyResult(R, Str);
+    std::string UniqueSuffix = (".$" + Str).str();
+    MangledName = MangledName + UniqueSuffix;
----------------
tmsriram wrote:
> pcc wrote:
> > Why `".$"` and not just `"."`?
> I just wanted to keep it consistent with what getUniqueModuleId does with the Md5 hash.  I can remove it.
Agreed. `"."` seems sufficient.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1118
+    std::string UniqueSuffix = (".$" + Str).str();
+    MangledName = MangledName + UniqueSuffix;
+  }
----------------
`MangledName += UniqueSuffix;`


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:966
                                          OPT_fno_unique_section_names, true);
+  Opts.UniqueInternalFuncNames = Args.hasFlag(
+      OPT_funique_internal_funcnames, OPT_fno_unique_internal_funcnames, false);
----------------
tmsriram wrote:
> MaskRay wrote:
> > Just `Args.hasArg(OPT_funique_internal_funcnames)`.
> > 
> > `-fno-*` is handled in clangDriver. CC1 does not need to know `-fno-*` if it defaults to false.
> 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.
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.


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

https://reviews.llvm.org/D73307