[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
Tue Mar 10 11:25:19 PDT 2020
DiggerLin marked 18 inline comments as done.
DiggerLin added inline comments.
Herald added a subscriber: MaskRay.
================
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:
> > 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.
please checked the patch. https://reviews.llvm.org/D74240
it compare the std::tuple<uint64_t, StringRef, uint8_t> and sort the symbol in the SectionSymbolsTy
the default less function of std:tuple is tie uint64_t, StringRef, uint8_t together and compare. we do not want to change the no xcoff test case for llvm-objdum -D, we keep to compare P1.Addr, P1.Name, P1.Type
================
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);
----------------
jhenderson wrote:
> 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.
for xcoff disassembly, if there are symbol have the same address. I think we do not care about which symbol to show when disassembly.
================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:17
StringRef XCOFF::getMappingClassString(XCOFF::StorageMappingClass SMC) {
switch (SMC) {
+ SMC_CASE(PR)
----------------
jhenderson wrote:
> 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.
There are several place to use getMappingClassString in the llvm(not only the patch). I change the interface of the function, We need to change several place(not related to this patch) in llvm. I will return a StringRef("Unknown"); here.
================
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:
> 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.
thanks let me know the usage. I will change it.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1177
+ return SymbolInfoTy(Addr, Name, Optional<XCOFF::StorageMappingClass>(),
+ Optional<uint32_t>(), false);
+ else
----------------
jhenderson wrote:
> I think you can use `None` instead of `Optional<uint32_t>()`.
thanks
================
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:
> 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.
I agree with you. But I think we maybe need to create separate NFC patch to address the problem.
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