[llvm] r192803 - Assert on duplicate registration. Don't depend on function pointer equality.
Sean Callanan
scallanan at apple.com
Wed Oct 16 13:06:28 PDT 2013
Rafael,
how do you reconcile this patch with the text on IntiializeAllTargetMCs() (llvm/Support/TargetSelect.h) that says specifically:
/// It is legal for a client to make multiple calls to this function.
LLDB in particular uses this because various of its subsystems depend on the TargetMCs being initialized and whoever gets to it first makes sure they are. We try to avoid static constructors.
I’d argue for reverting your patch as it breaks a declared interface contract.
Sean
On Oct 16, 2013, at 9:21 AM, Rafael Espindola <rafael.espindola at gmail.com> wrote:
> Author: rafael
> Date: Wed Oct 16 11:21:40 2013
> New Revision: 192803
>
> URL: http://llvm.org/viewvc/llvm-project?rev=192803&view=rev
> Log:
> Assert on duplicate registration. Don't depend on function pointer equality.
>
> Before this patch we would assert when building llvm as multiple shared
> libraries (cmake's BUILD_SHARED_LIBS). The problem was the line
>
> if (T.AsmStreamerCtorFn == Target::createDefaultAsmStreamer)
>
> which returns false because of -fvisibility-inlines-hidden. It is easy
> to fix just this one case, but I decided to try to also make the
> registration more strict. It looks like the old logic for ignoring
> followup registration was just a temporary hack that outlived its
> usefulness.
>
> This patch converts the ifs to asserts, fixes the few cases that were
> registering twice and makes sure all the asserts compare with null.
>
> Thanks for Joerg for reporting the problem and reviewing the patch.
>
> Modified:
> llvm/trunk/include/llvm/Support/TargetRegistry.h
> llvm/trunk/lib/Target/NVPTX/NVPTXTargetMachine.cpp
> llvm/trunk/unittests/ExecutionEngine/JIT/JITTest.cpp
>
> Modified: llvm/trunk/include/llvm/Support/TargetRegistry.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/TargetRegistry.h?rev=192803&r1=192802&r2=192803&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/TargetRegistry.h (original)
> +++ llvm/trunk/include/llvm/Support/TargetRegistry.h Wed Oct 16 11:21:40 2013
> @@ -235,22 +235,10 @@ namespace llvm {
> /// MCSymbolizer, if registered (default = llvm::createMCSymbolizer)
> MCSymbolizerCtorTy MCSymbolizerCtorFn;
>
> - static MCStreamer *
> - createDefaultAsmStreamer(MCContext &Ctx, formatted_raw_ostream &OS,
> - bool isVerboseAsm, bool useLoc, bool useCFI,
> - bool useDwarfDirectory, MCInstPrinter *InstPrint,
> - MCCodeEmitter *CE, MCAsmBackend *TAB,
> - bool ShowInst) {
> - return llvm::createAsmStreamer(Ctx, 0, OS, isVerboseAsm, useLoc, useCFI,
> - useDwarfDirectory, InstPrint, CE, TAB,
> - ShowInst);
> - }
> -
> public:
> Target()
> - : AsmStreamerCtorFn(createDefaultAsmStreamer),
> - MCRelocationInfoCtorFn(llvm::createMCRelocationInfo),
> - MCSymbolizerCtorFn(llvm::createMCSymbolizer) {}
> + : AsmStreamerCtorFn(0), MCRelocationInfoCtorFn(0),
> + MCSymbolizerCtorFn(0) {}
>
> /// @name Target Information
> /// @{
> @@ -452,9 +440,13 @@ namespace llvm {
> MCCodeEmitter *CE,
> MCAsmBackend *TAB,
> bool ShowInst) const {
> - // AsmStreamerCtorFn is default to llvm::createAsmStreamer
> - return AsmStreamerCtorFn(Ctx, OS, isVerboseAsm, useLoc, useCFI,
> - useDwarfDirectory, InstPrint, CE, TAB, ShowInst);
> + if (AsmStreamerCtorFn)
> + return AsmStreamerCtorFn(Ctx, OS, isVerboseAsm, useLoc, useCFI,
> + useDwarfDirectory, InstPrint, CE, TAB,
> + ShowInst);
> + return llvm::createAsmStreamer(Ctx, 0, OS, isVerboseAsm, useLoc, useCFI,
> + useDwarfDirectory, InstPrint, CE, TAB,
> + ShowInst);
> }
>
> /// createMCRelocationInfo - Create a target specific MCRelocationInfo.
> @@ -463,7 +455,10 @@ namespace llvm {
> /// \param Ctx The target context.
> MCRelocationInfo *
> createMCRelocationInfo(StringRef TT, MCContext &Ctx) const {
> - return MCRelocationInfoCtorFn(TT, Ctx);
> + MCRelocationInfoCtorTy Fn = MCRelocationInfoCtorFn
> + ? MCRelocationInfoCtorFn
> + : llvm::createMCRelocationInfo;
> + return Fn(TT, Ctx);
> }
>
> /// createMCSymbolizer - Create a target specific MCSymbolizer.
> @@ -480,8 +475,9 @@ namespace llvm {
> LLVMSymbolLookupCallback SymbolLookUp,
> void *DisInfo,
> MCContext *Ctx, MCRelocationInfo *RelInfo) const {
> - return MCSymbolizerCtorFn(TT, GetOpInfo, SymbolLookUp, DisInfo,
> - Ctx, RelInfo);
> + MCSymbolizerCtorTy Fn =
> + MCSymbolizerCtorFn ? MCSymbolizerCtorFn : llvm::createMCSymbolizer;
> + return Fn(TT, GetOpInfo, SymbolLookUp, DisInfo, Ctx, RelInfo);
> }
>
> /// @}
> @@ -602,9 +598,8 @@ namespace llvm {
> /// @param T - The target being registered.
> /// @param Fn - A function to construct a MCAsmInfo for the target.
> static void RegisterMCAsmInfo(Target &T, Target::MCAsmInfoCtorFnTy Fn) {
> - // Ignore duplicate registration.
> - if (!T.MCAsmInfoCtorFn)
> - T.MCAsmInfoCtorFn = Fn;
> + assert(!T.MCAsmInfoCtorFn);
> + T.MCAsmInfoCtorFn = Fn;
> }
>
> /// RegisterMCCodeGenInfo - Register a MCCodeGenInfo implementation for the
> @@ -618,9 +613,8 @@ namespace llvm {
> /// @param Fn - A function to construct a MCCodeGenInfo for the target.
> static void RegisterMCCodeGenInfo(Target &T,
> Target::MCCodeGenInfoCtorFnTy Fn) {
> - // Ignore duplicate registration.
> - if (!T.MCCodeGenInfoCtorFn)
> - T.MCCodeGenInfoCtorFn = Fn;
> + assert(!T.MCCodeGenInfoCtorFn);
> + T.MCCodeGenInfoCtorFn = Fn;
> }
>
> /// RegisterMCInstrInfo - Register a MCInstrInfo implementation for the
> @@ -633,18 +627,16 @@ namespace llvm {
> /// @param T - The target being registered.
> /// @param Fn - A function to construct a MCInstrInfo for the target.
> static void RegisterMCInstrInfo(Target &T, Target::MCInstrInfoCtorFnTy Fn) {
> - // Ignore duplicate registration.
> - if (!T.MCInstrInfoCtorFn)
> - T.MCInstrInfoCtorFn = Fn;
> + assert(!T.MCInstrInfoCtorFn);
> + T.MCInstrInfoCtorFn = Fn;
> }
>
> /// RegisterMCInstrAnalysis - Register a MCInstrAnalysis implementation for
> /// the given target.
> static void RegisterMCInstrAnalysis(Target &T,
> Target::MCInstrAnalysisCtorFnTy Fn) {
> - // Ignore duplicate registration.
> - if (!T.MCInstrAnalysisCtorFn)
> - T.MCInstrAnalysisCtorFn = Fn;
> + assert(!T.MCInstrAnalysisCtorFn);
> + T.MCInstrAnalysisCtorFn = Fn;
> }
>
> /// RegisterMCRegInfo - Register a MCRegisterInfo implementation for the
> @@ -657,9 +649,8 @@ namespace llvm {
> /// @param T - The target being registered.
> /// @param Fn - A function to construct a MCRegisterInfo for the target.
> static void RegisterMCRegInfo(Target &T, Target::MCRegInfoCtorFnTy Fn) {
> - // Ignore duplicate registration.
> - if (!T.MCRegInfoCtorFn)
> - T.MCRegInfoCtorFn = Fn;
> + assert(!T.MCRegInfoCtorFn);
> + T.MCRegInfoCtorFn = Fn;
> }
>
> /// RegisterMCSubtargetInfo - Register a MCSubtargetInfo implementation for
> @@ -673,9 +664,8 @@ namespace llvm {
> /// @param Fn - A function to construct a MCSubtargetInfo for the target.
> static void RegisterMCSubtargetInfo(Target &T,
> Target::MCSubtargetInfoCtorFnTy Fn) {
> - // Ignore duplicate registration.
> - if (!T.MCSubtargetInfoCtorFn)
> - T.MCSubtargetInfoCtorFn = Fn;
> + assert(!T.MCSubtargetInfoCtorFn);
> + T.MCSubtargetInfoCtorFn = Fn;
> }
>
> /// RegisterTargetMachine - Register a TargetMachine implementation for the
> @@ -689,9 +679,8 @@ namespace llvm {
> /// @param Fn - A function to construct a TargetMachine for the target.
> static void RegisterTargetMachine(Target &T,
> Target::TargetMachineCtorTy Fn) {
> - // Ignore duplicate registration.
> - if (!T.TargetMachineCtorFn)
> - T.TargetMachineCtorFn = Fn;
> + assert(!T.TargetMachineCtorFn);
> + T.TargetMachineCtorFn = Fn;
> }
>
> /// RegisterMCAsmBackend - Register a MCAsmBackend implementation for the
> @@ -704,8 +693,8 @@ namespace llvm {
> /// @param T - The target being registered.
> /// @param Fn - A function to construct an AsmBackend for the target.
> static void RegisterMCAsmBackend(Target &T, Target::MCAsmBackendCtorTy Fn) {
> - if (!T.MCAsmBackendCtorFn)
> - T.MCAsmBackendCtorFn = Fn;
> + assert(!T.MCAsmBackendCtorFn);
> + T.MCAsmBackendCtorFn = Fn;
> }
>
> /// RegisterMCAsmParser - Register a MCTargetAsmParser implementation for
> @@ -718,8 +707,8 @@ namespace llvm {
> /// @param T - The target being registered.
> /// @param Fn - A function to construct an MCTargetAsmParser for the target.
> static void RegisterMCAsmParser(Target &T, Target::MCAsmParserCtorTy Fn) {
> - if (!T.MCAsmParserCtorFn)
> - T.MCAsmParserCtorFn = Fn;
> + assert(!T.MCAsmParserCtorFn);
> + T.MCAsmParserCtorFn = Fn;
> }
>
> /// RegisterAsmPrinter - Register an AsmPrinter implementation for the given
> @@ -732,9 +721,8 @@ namespace llvm {
> /// @param T - The target being registered.
> /// @param Fn - A function to construct an AsmPrinter for the target.
> static void RegisterAsmPrinter(Target &T, Target::AsmPrinterCtorTy Fn) {
> - // Ignore duplicate registration.
> - if (!T.AsmPrinterCtorFn)
> - T.AsmPrinterCtorFn = Fn;
> + assert(!T.AsmPrinterCtorFn);
> + T.AsmPrinterCtorFn = Fn;
> }
>
> /// RegisterMCDisassembler - Register a MCDisassembler implementation for
> @@ -748,8 +736,8 @@ namespace llvm {
> /// @param Fn - A function to construct an MCDisassembler for the target.
> static void RegisterMCDisassembler(Target &T,
> Target::MCDisassemblerCtorTy Fn) {
> - if (!T.MCDisassemblerCtorFn)
> - T.MCDisassemblerCtorFn = Fn;
> + assert(!T.MCDisassemblerCtorFn);
> + T.MCDisassemblerCtorFn = Fn;
> }
>
> /// RegisterMCInstPrinter - Register a MCInstPrinter implementation for the
> @@ -763,8 +751,8 @@ namespace llvm {
> /// @param Fn - A function to construct an MCInstPrinter for the target.
> static void RegisterMCInstPrinter(Target &T,
> Target::MCInstPrinterCtorTy Fn) {
> - if (!T.MCInstPrinterCtorFn)
> - T.MCInstPrinterCtorFn = Fn;
> + assert(!T.MCInstPrinterCtorFn);
> + T.MCInstPrinterCtorFn = Fn;
> }
>
> /// RegisterMCCodeEmitter - Register a MCCodeEmitter implementation for the
> @@ -778,8 +766,8 @@ namespace llvm {
> /// @param Fn - A function to construct an MCCodeEmitter for the target.
> static void RegisterMCCodeEmitter(Target &T,
> Target::MCCodeEmitterCtorTy Fn) {
> - if (!T.MCCodeEmitterCtorFn)
> - T.MCCodeEmitterCtorFn = Fn;
> + assert(!T.MCCodeEmitterCtorFn);
> + T.MCCodeEmitterCtorFn = Fn;
> }
>
> /// RegisterMCObjectStreamer - Register a object code MCStreamer
> @@ -793,8 +781,8 @@ namespace llvm {
> /// @param Fn - A function to construct an MCStreamer for the target.
> static void RegisterMCObjectStreamer(Target &T,
> Target::MCObjectStreamerCtorTy Fn) {
> - if (!T.MCObjectStreamerCtorFn)
> - T.MCObjectStreamerCtorFn = Fn;
> + assert(!T.MCObjectStreamerCtorFn);
> + T.MCObjectStreamerCtorFn = Fn;
> }
>
> /// RegisterAsmStreamer - Register an assembly MCStreamer implementation
> @@ -807,8 +795,8 @@ namespace llvm {
> /// @param T - The target being registered.
> /// @param Fn - A function to construct an MCStreamer for the target.
> static void RegisterAsmStreamer(Target &T, Target::AsmStreamerCtorTy Fn) {
> - if (T.AsmStreamerCtorFn == Target::createDefaultAsmStreamer)
> - T.AsmStreamerCtorFn = Fn;
> + assert(!T.AsmStreamerCtorFn);
> + T.AsmStreamerCtorFn = Fn;
> }
>
> /// RegisterMCRelocationInfo - Register an MCRelocationInfo
> @@ -822,8 +810,8 @@ namespace llvm {
> /// @param Fn - A function to construct an MCRelocationInfo for the target.
> static void RegisterMCRelocationInfo(Target &T,
> Target::MCRelocationInfoCtorTy Fn) {
> - if (T.MCRelocationInfoCtorFn == llvm::createMCRelocationInfo)
> - T.MCRelocationInfoCtorFn = Fn;
> + assert(!T.MCRelocationInfoCtorFn);
> + T.MCRelocationInfoCtorFn = Fn;
> }
>
> /// RegisterMCSymbolizer - Register an MCSymbolizer
> @@ -837,8 +825,8 @@ namespace llvm {
> /// @param Fn - A function to construct an MCSymbolizer for the target.
> static void RegisterMCSymbolizer(Target &T,
> Target::MCSymbolizerCtorTy Fn) {
> - if (T.MCSymbolizerCtorFn == llvm::createMCSymbolizer)
> - T.MCSymbolizerCtorFn = Fn;
> + assert(!T.MCSymbolizerCtorFn);
> + T.MCSymbolizerCtorFn = Fn;
> }
>
> /// @}
>
> Modified: llvm/trunk/lib/Target/NVPTX/NVPTXTargetMachine.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/NVPTX/NVPTXTargetMachine.cpp?rev=192803&r1=192802&r2=192803&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/NVPTX/NVPTXTargetMachine.cpp (original)
> +++ llvm/trunk/lib/Target/NVPTX/NVPTXTargetMachine.cpp Wed Oct 16 11:21:40 2013
> @@ -57,9 +57,6 @@ extern "C" void LLVMInitializeNVPTXTarge
> RegisterTargetMachine<NVPTXTargetMachine32> X(TheNVPTXTarget32);
> RegisterTargetMachine<NVPTXTargetMachine64> Y(TheNVPTXTarget64);
>
> - RegisterMCAsmInfo<NVPTXMCAsmInfo> A(TheNVPTXTarget32);
> - RegisterMCAsmInfo<NVPTXMCAsmInfo> B(TheNVPTXTarget64);
> -
> // FIXME: This pass is really intended to be invoked during IR optimization,
> // but it's very NVPTX-specific.
> initializeNVVMReflectPass(*PassRegistry::getPassRegistry());
>
> Modified: llvm/trunk/unittests/ExecutionEngine/JIT/JITTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ExecutionEngine/JIT/JITTest.cpp?rev=192803&r1=192802&r2=192803&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/ExecutionEngine/JIT/JITTest.cpp (original)
> +++ llvm/trunk/unittests/ExecutionEngine/JIT/JITTest.cpp Wed Oct 16 11:21:40 2013
> @@ -721,16 +721,4 @@ TEST(LazyLoadedJITTest, EagerCompiledRec
> }
> #endif // !defined(__arm__) && !defined(__powerpc__) && !defined(__s390__)
>
> -// This code is copied from JITEventListenerTest, but it only runs once for all
> -// the tests in this directory. Everything seems fine, but that's strange
> -// behavior.
> -class JITEnvironment : public testing::Environment {
> - virtual void SetUp() {
> - // Required to create a JIT.
> - InitializeNativeTarget();
> - }
> -};
> -testing::Environment* const jit_env =
> - testing::AddGlobalTestEnvironment(new JITEnvironment);
> -
> }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131016/01822911/attachment.html>
More information about the llvm-commits
mailing list