This is a preparation for ARM64EC/ARM64X binaries, which may contain both ARM64 and x86_64 code in the same file. llvm-objdump already has partial support for mixing disassemblers for ARM thumb mode support. However, for ARM64EC we can't share MCContext, MCInstrAnalysis and PrettyPrinter instances. This patch provides additional abstraction which makes adding mixed code support later in the series easier.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Largely looks okay to me. Just a few small points.
| llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
|---|---|---|
| 1304 | Perhaps best to call this TheTarget here, to avoid unusual syntax like const class *Target. | |
| 1313 | These should be StringRef or string_view right, not passed-by-value std::string? | |
| 1332 | This and some of the other code can move to the initialiser list, right? | |
| 1353–1356 | This feels a little weird to me (having two pointer members essentially to the same thing), although I see why you've done it. Did you consider using a shared_ptr instead? That also avoids coupling the secondary target quite so much to the primary target, so destruction order is less important. | |
| 1385 | This can all be done in the initialiser list, rather than the constructor body, if I'm not mistaken. | |
Comment Actions
Thanks. The new version renames Target to TheTarget, uses StringRef for constructor arguments, uses initialiser lists for more members. It also uses std::shared_ptr for members that may be shared among instances.
| llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
|---|---|---|
| 1302–1402 | The class should be in anonymous namespace. | |
Comment Actions
The new version uses get() and moves the class to anonymous namespace. Thanks for reviews.
| llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
|---|---|---|
| 837–838 | As an alternative to my suggested inline edit, you could add a function that creates the relevant smart pointer and checks for the error before returning the pointer. You could then move most/all of the body of this class into the initialiser list. Something like the following rough code: template <typename PtrTy, typename... ArgsTy>
PtrTy makePtrOrError(std::function<PtrTy(ArgsTy...)> Func, StringRef Message, ArgsTy... Args) {
PtrTy Ptr = Func(Args...);
if (!Ptr)
reportError(Message);
return Ptr;
}
DisassemblerTarget::DisassemblerTarget(...)
: ...
RegisterInfo(makePtrOrError(...)), AsmInfo(makePtrOrError(...)), /*etc*/{} | |
| 895 | Nit: don't need the new line at the end of the namespace IMHO. | |
As an alternative to my suggested inline edit, you could add a function that creates the relevant smart pointer and checks for the error before returning the pointer. You could then move most/all of the body of this class into the initialiser list. Something like the following rough code:
template <typename PtrTy, typename... ArgsTy> PtrTy makePtrOrError(std::function<PtrTy(ArgsTy...)> Func, StringRef Message, ArgsTy... Args) { PtrTy Ptr = Func(Args...); if (!Ptr) reportError(Message); return Ptr; } DisassemblerTarget::DisassemblerTarget(...) : ... RegisterInfo(makePtrOrError(...)), AsmInfo(makePtrOrError(...)), /*etc*/{}