[PATCH] D53736: [BTF] Add BTF DebugInfo

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added a comment.

In https://reviews.llvm.org/D53736#1300772, @yonghong-song wrote:

> @aprantl I just tried. The following is the change which works by using a stub BPFDebug when BPF target is not specified.
>
>   diff --git a/lib/CodeGen/AsmPrinter/BTFDebugStub.cpp b/lib/CodeGen/AsmPrinter/BTFDebugStub.cpp
>   new file mode 100644
>   index 00000000000..f27e9c5c75f
>   --- /dev/null
>   +++ b/lib/CodeGen/AsmPrinter/BTFDebugStub.cpp
>   @@ -0,0 +1,46 @@
>   +//===- llvm/lib/CodeGen/AsmPrinter/BTFDebugStub.cpp -------------*- C++ -*-===//
>   +//
>   +//                     The LLVM Compiler Infrastructure
>   +//
>   +// This file is distributed under the University of Illinois Open Source
>   +// License. See LICENSE.TXT for details.
>   +//
>   +//===----------------------------------------------------------------------===//
>   +///
>   +/// \file
>   +/// This file is a stub for BTF generation support. The real implementation
>   +/// is at BTFDebug.cpp which will be included if BPF target is built.
>   +///
>   +//===----------------------------------------------------------------------===//
>   +
>   +#include "DebugHandlerBase.h"
>   +
>   +namespace llvm {
>   +
>   +/// Collect and emit BTF information.
>   +class BTFDebug : public DebugHandlerBase {
>   +
>   +protected:
>   +  /// Gather pre-function debug information.
>   +  void beginFunctionImpl(const MachineFunction *MF) override {};
>   +
>   +  /// Post process after all instructions in this function are processed.
>   +  void endFunctionImpl(const MachineFunction *MF) override {};
>   +
>   +public:
>   +  BTFDebug(AsmPrinter *AP);
>   +
>   +  void setSymbolSize(const MCSymbol *Symbol, uint64_t Size) override {}
>   +
>   +  /// Process beginning of an instruction.
>   +  void beginInstruction(const MachineInstr *MI) override {
>   +    DebugHandlerBase::beginInstruction(MI);
>   +  };
>   +
>   +  /// Complete all the types and emit the BTF sections.
>   +  void endModule() override {};
>   +};
>   +
>   +BTFDebug::BTFDebug(AsmPrinter *AP) : DebugHandlerBase(AP) {}
>   +
>   +} // end namespace llvm
>   diff --git a/lib/CodeGen/AsmPrinter/CMakeLists.txt b/lib/CodeGen/AsmPrinter/CMakeLists.txt
>   index 24c5d33a120..e12fbf92c77 100644
>   --- a/lib/CodeGen/AsmPrinter/CMakeLists.txt
>   +++ b/lib/CodeGen/AsmPrinter/CMakeLists.txt
>   @@ -1,3 +1,14 @@
>   +# depends on whether BPF target is built or not
>   +# eitheer BTFDebug.cpp or BTFDebugStub.cpp is included,
>   +# but not both.
>   +set(LLVM_OPTIONAL_SOURCES "BTFDebug.cpp" "BTFDebugStub.cpp")
>   +list(FIND LLVM_TARGETS_TO_BUILD "BPF" idx)
>   +if( NOT idx LESS 0 )
>   +set(BTFDebug_File "BTFDebug.cpp")
>   +else()
>   +set(BTFDebug_File "BTFDebugStub.cpp")
>   +endif()
>   +
>    add_llvm_library(LLVMAsmPrinter
>      AccelTable.cpp
>      AddressPool.cpp
>   @@ -24,7 +35,7 @@ add_llvm_library(LLVMAsmPrinter
>      WinException.cpp
>      CodeViewDebug.cpp
>      WasmException.cpp
>   -  BTFDebug.cpp
>   +  ${BTFDebug_File}
>   
>      DEPENDS
>      intrinsics_gen
>
>
> This adds complexity to make file and it adds one more file BTFDebugStub.cpp. I personally think this is not a good approach.
>  But please let me know what you think and I can implement this if you think it is good.


LLVM code size is an actual problem, and since this code only makes sense when the BTF target is present, I think that we should make sure that it's only there when it's needed. Since you don't seem too happy with detecting the BTF target in the AsmPrinter CMake file, perhaps defining the interface in AsmPrinter, moving the implementation to the target directory and having a unique_ptr to the implementation in AsmPrinter would be more elegant?


Repository:
  rL LLVM

https://reviews.llvm.org/D53736