[PATCH] D72973: using symbol index+symbol name + storage mapping class as label for llvm-objdump -D
Digger via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 2 12:06:53 PST 2020
DiggerLin marked 24 inline comments as done and an inline comment as not done.
DiggerLin added inline comments.
================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:26
+ uint64_t Index;
+ bool ShowSymDesc;
+ XCOFFSymbolInfo(Optional<XCOFF::StorageMappingClass> Smc, uint64_t Idx,
----------------
jasonliu wrote:
> I wonder if this bool is really necessary. Could you instead check if it is label in getXCOFFSymbolCsectSMC? If it is a label, then return the empty Optional.
> In other words, is it necessary to get the SMC from a label at the first place?
I understood your suggestion. but Label also has the storage mapping class. from the function name we know that the function getXCOFFSymbolCsectSMC get the Storage mapping class for the all the symbol which has it. if it will do not return the Storage mapping class for the label. I think I have to change the function name to reflect it ? I am prefer to keep current implement. @hubert.reinterpretcast , what do you think about it ?
================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:47
+ friend bool operator<(const SymbolInfoTy &P1, const SymbolInfoTy &P2) {
+ return std::tie(P1.Addr, P1.Name, P1.Type) <
+ std::tie(P2.Addr, P2.Name, P2.Type);
----------------
jasonliu wrote:
> What if you are comparing a SymbolInfoTy which have XCOFFSymbolInfo instead of Type?
1. for the XCOFF , as we discuss before, for symbols which has the same address , we do not care about the which symbol we pick up for the llvm-objdump -D .
2. we do not want to change the elf related test case .so we use use Addr, Name, Type to compare.
3. when you look into the Optional class data structure, I think we compare storage mapping class actually.
================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:47
+ friend bool operator<(const SymbolInfoTy &P1, const SymbolInfoTy &P2) {
+ return std::tie(P1.Addr, P1.Name, P1.Type) <
+ std::tie(P2.Addr, P2.Name, P2.Type);
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jasonliu wrote:
> > > What if you are comparing a SymbolInfoTy which have XCOFFSymbolInfo instead of Type?
> > 1. for the XCOFF , as we discuss before, for symbols which has the same address , we do not care about the which symbol we pick up for the llvm-objdump -D .
> >
> > 2. we do not want to change the elf related test case .so we use use Addr, Name, Type to compare.
> >
> > 3. when you look into the Optional class data structure, I think we compare storage mapping class actually.
> >
> This has been marked as "Done" but I don't see where?
I have explain why I use P1.Addr, P1.Name, P1.Type to compare in my comment, So I marked it done.
================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:17
StringRef XCOFF::getMappingClassString(XCOFF::StorageMappingClass SMC) {
switch (SMC) {
+ SMC_CASE(PR)
----------------
jhenderson wrote:
> If I'm not mistaken, this function could end up not hitting a return if the `SMC` is not a recognised enum. It needs handling in some way (e.g. returning `StringRef()` or a string representing an unknown value such as `to_string(SMC)`).
I added report_fatal_error("Unhandled storage-mapping class.");
outside the switch { }.
the function getMappingClassString() converts all the enum item in the XCOFF::StorageMappingClass currently. if someone add a new SMC enum item, it will hit report_fatal_error() and the developer need to add here corresponding .
================
Comment at: llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test:5
+# RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
+# RUN: FileCheck --check-prefix=CHECK-GNU %s
+
----------------
jhenderson wrote:
> Indent continuation lines by a couple of spaces for readability.
>
> ```
> # RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o | \
> # RUN: FileCheck --check-prefix=CHECK-GNU %s
> ```
>
> Also, I think `CHECK` should be the default without `--symbol-description` and `CHECK-DESC` or something with the option, or better yet, add it to a new test file. Also, why are you checking `--symbol-description` with `--disassemble-all/-D` instead of plain disassembly (i.e. `--disassemble`).
>
> Finally, you probably need a test case + maybe a warning for using `--symbol-description` with something that isn't XCOFF.
I do not saw different of "-D" "--disassemble-all", and "--disassemble" in the llvm-objdump --help
and
the original test case has
RUN: llvm-objdump -D %p/Inputs/xcoff-section-headers.o
I just added a new option --symbol-description in the this test case and test the new option. I think keep the original option of test case llvm-objdump -D is a better choice.
I am prefer to put the test without --symbol-description in current test case.
we can know what is the different output of llvm-objdump -D with and without
--symbol-description at a glance.
================
Comment at: llvm/test/tools/llvm-objdump/xcoff-disassemble-all.test:59-61
+
+
+
----------------
jhenderson wrote:
> Why so many blank lines?
it just for the separate different test .I delete some blank lines
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:64
+void printXCOFFSymbolDescription(SymbolInfoTy &SymbolInfo,
+ std::string &SymbolName) {
+
----------------
jhenderson wrote:
> `SymbolName` isn't modified, so make this a `StringRef`.
the function is only use as
if (Obj->isXCOFF() && SymbolDescription) {
printXCOFFSymbolDescription(Symbols[SI], SymbolName);
outs() << ":\n";
} else
outs() << SymbolName << ":\n";
the SymbolName is define as
std::string SymbolName = Symbols[SI].Name.str();
if (Demangle)
SymbolName = demangle(SymbolName);
So I think using void printXCOFFSymbolDescription(SymbolInfoTy &SymbolInfo,
const std::string &SymbolName) is better.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:67
+ const int64_t SymIndex = SymbolInfo.XCOFFSymInfo.Index;
+ if (SymIndex >= 0)
+ outs() << "(idx: " << SymIndex << ") ";
----------------
jhenderson wrote:
> When can this be less than zero?
dummy symbol do not have Symbol index, we put -1 as symbol index .
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:77
+ SymbolInfo.XCOFFSymInfo.StorageMappingClass.getValue();
+ if (SymIndex >= 0 && SymbolInfo.XCOFFSymInfo.ShowSymDesc)
+ outs() << "[" << XCOFF::getMappingClassString(Smc).str() << "]";
----------------
jhenderson wrote:
> Seems like `ShowSymDesc` should be an argument of this function rather than a property of the symbol?
change the ShowSymDesc to IsLabel
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1173
+ return SymbolInfoTy(Addr, Name, Optional<XCOFF::StorageMappingClass>(),
+ No_Sym_Index, false);
+ else
----------------
jhenderson wrote:
> How about making the symbol index an `Optional`? That way you don't need a magic number/named constant.
done
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.h:150-157
+getXCOFFSymbolCsectSMC(const object::XCOFFObjectFile *Obj,
+ const object::SymbolRef &Sym);
+bool isSymbolDescriptionDisplay(const object::ObjectFile *Obj,
+ const object::SymbolRef &Sym);
+int64_t getSymbolIndex(const object::ObjectFile *Obj,
+ const object::SymbolRef &Sym);
+void printXCOFFSymbolDescription(SymbolInfoTy &SymbolInfo,
----------------
jhenderson wrote:
> These are all defined in XCOFFDump.cpp, so put the declarations in XCOFFDump.h.
There are something as following in this header file.
void parseInputMachO(object::MachOUniversalBinary *UB);
void printCOFFUnwindInfo(const object::COFFObjectFile *O);
void printMachOUnwindInfo(const object::MachOObjectFile *O);
void printMachOExportsTrie(const object::MachOObjectFile *O);
void printMachORebaseTable(object::MachOObjectFile *O);
void printMachOBindTable(object::MachOObjectFile *O);
void printMachOLazyBindTable(object::MachOObjectFile *O);
void printMachOWeakBindTable(object::MachOObjectFile *O);
I do not think I need to put the new declare function into a new XCOFFDump.h
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72973/new/
https://reviews.llvm.org/D72973
More information about the llvm-commits
mailing list