[PATCH] D72973: using symbol index+symbol name + storage mapping class as label for llvm-objdump -D
James Henderson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 4 02:20:30 PST 2020
jhenderson added inline comments.
================
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);
----------------
DiggerLin wrote:
> 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.
> we do not want to change the elf related test case .so we use use Addr, Name, Type to compare.
As noted above, two ELF symbols can be identical, but be distinct symbols. Do you care about this situation?
> when you look into the Optional class data structure, I think we compare storage mapping class actually.
But you don't compare using the XCOFFSymInfo member. I don't think you can assume anything about the layout of the union, so cannot assume that comparing using `Type` will work if it has been initialised the other way.
================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:35
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);
+ return std::tie(P1.Addr, P1.Name, P1.TypeOrSmc) <
+ std::tie(P2.Addr, P2.Name, P2.TypeOrSmc);
----------------
DiggerLin wrote:
> jasonliu wrote:
> > Should we add member "Index" here?
> I do not think there is any symbol that has the same the symbol address , symbol name and symbol type with different symbol index. So we do not need to add Index here
Are you talking about XCOFF here? For ELF, it is possible for this to happen. Take two source files as follows:
```
// bar.c
static int bar() { return 42; }
int foo();
int _start() { return bar() + foo(); }
// foo.c
static int bar() { return 42; }
int foo() { return bar(); }
```
Compile using -ffunction-sections, and link them together using --icf=all, and you end up with two local symbols called `bar`, with the same address and type.
================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:17
StringRef XCOFF::getMappingClassString(XCOFF::StorageMappingClass SMC) {
switch (SMC) {
+ SMC_CASE(PR)
----------------
DiggerLin wrote:
> 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 .
Please don't use `report_fatal_error` if at all possible. This produces error messages that imply that it is an internal error (something similar to an assert, but works if assertions are disabled). You can change `getMappingClassString` to return either an empty `StringRef()` or an `Expected<StringRef>` and report an error properly.
If somebody has defined their SMC field to not be one of the recognised values, you want the dumping code to handle it nicely, without hitting `report_fatal_error` or a crash...
Please also add a test case for this situation.
================
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
+
----------------
DiggerLin wrote:
> DiggerLin wrote:
> > 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.
> >
> >
> I just read the source code, there are different of "--disassemble-all", and "--disassemble".
> "--disassemble" only disassembly text section. "--disassemble-all" disassembly all sections
>
> and in the test case , it also disassembly .data section, I think we need to use "--disassemble-all" or '-D" for .data section.
For ELF, --disassemble disassembles all sections with the SHF_EXECINSTR flag, i.e. things like .text etc, whereas --disassemble-all disassembles non executable sections too. I suppose it is okay to add it here. However, the biggest benefit would be to combine the two sets of CHECKs using --check-prefixes:
```
# RUN: <regular -D> | FileCheck %s --check-prefixes=COMMON,PLAIN
# RUN: <with --symbol-description> | FileCheck %s --check-prefixes=COMMON,DESC
# COMMON: Inputs/xcoff-section-headers.o: file format aixcoff-rs6000
# COMMON: Disassembly of section .text:
# PLAIN: 00000000 .text:
# DESC: 00000000 (idx: 4) .text:
# COMMON-NEXT: 0: 80 62 00 04 lwz 3, 4(2)
...
```
You haven't addressed my indentation comment either.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:64
+void printXCOFFSymbolDescription(SymbolInfoTy &SymbolInfo,
+ std::string &SymbolName) {
+
----------------
DiggerLin wrote:
> 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.
>
`StringRef` is intended to be used instead of `const std::string &` as it supports both `const char *` and `const std::string &` with less heap allocation. See the [[ http://llvm.org/docs/ProgrammersManual.html#passing-strings-the-stringref-and-twine-classes | Programmer's Manual ]].
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:149
+ SymbolDescription("symbol-description",
+ cl::desc("Add symbol description for dissembly"),
+ cl::init(false), cl::cat(ObjdumpCat));
----------------
jhenderson wrote:
> jhenderson wrote:
> > hubert.reinterpretcast wrote:
> > > s/dissembly/disassembly/;
> > Please add the new switch to the CommandGuide documentation. It probably also needs an example in that doc.
> >
> > Also, if this opton is XCOFF-specific, please say so in the description.
> > Please add the new switch to the CommandGuide documentation. It probably also needs an example in that doc.
>
> This hasn't been addressed.
This still hasn't been addressed. Do you need any assistance?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:149-150
+ SymbolDescription("symbol-description",
+ cl::desc("Add symbol description for disassembly, This "
+ "option is for XCOFF-only."),
+ cl::init(false), cl::cat(ObjdumpCat));
----------------
jhenderson wrote:
> "Add symbol description for disassembly. This option is for XCOFF files only."
This hasn't been properly addressed. ',' -> '.' before the "This".
Also, please remove the trailing full stop in the help description, to match other options.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1177
+ return SymbolInfoTy(Addr, Name, Optional<XCOFF::StorageMappingClass>(),
+ Optional<uint32_t>(), false);
+ else
----------------
I think you can use `None` instead of `Optional<uint32_t>()`.
================
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,
----------------
DiggerLin wrote:
> 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
Those should be refactored into separate files too. Let's not compound bad form by blindly following what was done before.
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