tahonermann added inline comments.
================ Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6348-6349 + } else { + // If neither of these things, we have a user we don't know how to handle, + // so default to previous behavior of emitting a terrible error message. + return false; ---------------- This comment can only be understood in the context of this commit with the FIXME comment noted in the relevant test case. I suggest: // This user is one we don't know how to handle, so fail redirection. This // will result in an ifunc retaining a resolver name that will ultimately fail // to be resolved to a defined function. ================ Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6390-6391 llvm::GlobalValue *Val = I.second; - if (Val && !getModule().getNamedValue(Name->getName())) + llvm::GlobalValue *ExistingElem = + getModule().getNamedValue(Name->getName()); + ---------------- I suggest moving the declaration of `ExistingElem` after the break on `!Val` given that the former is not relevant if `Val` is null. I think the `!Val` check is worth a comment here. I suggest: // If Val is null, that implies that there were multiple declarations that each // had a claim to the unmangled name. In this case, generation of the alias // is suppressed. See CodeGenModule::MaybeHandleStaticInExternC(). The lack of checking for an existing element with the desired alias name was a pre-existing oversight, yes? ================ Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6396-6397 + + // Simple case, where there are no ifuncs involved. + if (!ExistingElem || CheckAndReplaceExternCIFuncs(ExistingElem, Val)) addCompilerUsedGlobal(llvm::GlobalAlias::create(Name->getName(), Val)); ---------------- This comment seems inconsistent with the code. If there are no ifuncs involved, then why call `CheckAndReplaceExternCIFuncs()`? ================ Comment at: clang/lib/CodeGen/CodeGenModule.h:1573-1575 + /// Helper function for EmitStaticExternCAliases that clears the uses of + /// 'Elem' if it is used exclusively by ifunc resolvers. Returns 'true' if it + /// was successful erases Elem. ---------------- Grammar is off in the last sentence. The comment doesn't really explain this function's purpose. I suggest: /// Helper function for EmitStaticExternCAliases() to redirect ifuncs that have a resolver /// name that matches 'Elem' to instead resolve to the name of 'CppFunc'. This /// redirection is necessary in cases where 'Elem' has a name that will be emitted as /// an alias of the name bound to 'CppFunc'; ifuncs may not reference aliases. Redirection /// is only performed if 'Elem' is only used by ifuncs in which case, 'Elem' is destroyed.. /// 'true' is returned If redirection is successful, and 'false' is returned otherwise. ================ Comment at: clang/test/SemaCXX/externc-ifunc-resolver.cpp:9-13 +// FIXME: This diagnostic is pretty confusing, the issue is that the existence +// of the two functions suppresses the 'alias' creation, and thus the ifunc +// resolution via the alias as well. In the future we should probably find +// some way to improve this diagnostic (likely by diagnosing when we decide +// this case suppresses alias creation). ---------------- Thanks for adding this comment; it is helpful. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122608/new/ https://reviews.llvm.org/D122608 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits