[PATCH] D96109: Refactor implementation of -funique-internal-linkage-names.

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 18 13:23:29 PST 2021


dblaikie added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5561
+  // the mangler will not do the right thing without one.
+  if (D->getKind() == Decl::Function && !getLangOpts().CPlusPlus &&
+      cast<FunctionDecl>(D)->getType()->getAs<FunctionProtoType>() &&
----------------
aaron.ballman wrote:
> tmsriram wrote:
> > tmsriram wrote:
> > > dblaikie wrote:
> > > > aaron.ballman wrote:
> > > > > It's a bit novel to add the attribute at codegen time rather than at sema -- is there a reason this attribute should not be added to the AST from within Sema?
> > > > How would it be if this didn't use an attribute at all? If this checking was done in ItaniumMangle directly, rather than checking done here, transforming the result of the checking into an attribute, and then checking the attribute later?
> > > @dblaikie  That is what I tried to do first.  I didnt find an easy way to do this without either storing some state in the Mangler about every such decl or adding something to the decl itself.  I guess the Mangler needs to know 3 things:
> > > 
> > > 1) That funique-internal-linakge-names is enabled - maybe I can add a bool
> > > 2) That the language is not C++ - This is already possible
> > > 3) That the decl has internal linkage - this may require adding more state to the mangler
> > > 
> > > I think it is 3) that would require some plumbing unless I missed something already existing.
> > I will revisit this, at a high level I think the mangler should know about the linkage of the decl as it  does "_ZL" for this. I think I missed it.
> > How would it be if this didn't use an attribute at all? If this checking was done in ItaniumMangle directly, rather than checking done here, transforming the result of the checking into an attribute, and then checking the attribute later?
> 
> I think that would have the same oddity, wouldn't it? However, I didn't do a good job of explaining my concern, which may not actually be a valid concern: would someone ever want to use this information from, say, an AST matcher for a clang-tidy check? Or is this purely a way to plumb information through the compiler without adding extra bits on a `FunctionDecl`? My concern was mostly that someone may want to do an AST match on this and they won't be able to because the attribute is added too late in the process.
> 
> We currently use 40 bits within `FunctionDeclBitfields`, so adding a single bit there does not seem like it would be the end of the world (but I didn't check thoroughly for issues with adding another bit there), so that's an option for squirreling the information away without something as heavy as an attribute.
the mangler has the clang::Function, so I'd have thought it could figure out the linkage from that - but I realize the linkage calculations are a bit more complicated/probably need some other sources


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

https://reviews.llvm.org/D96109



More information about the llvm-commits mailing list