[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
Mon Feb 24 02:06:23 PST 2020
jhenderson requested changes to this revision.
jhenderson added a comment.
This revision now requires changes to proceed.
Please don't just blanket-paste the new option everywhere where it might change behaviour. Instead, write one or more dedicated tests that show the behaviour of the new option. If there's interesting overlap between this and another option, you might want more additional tests, but the behaviour without the new option is still valid behaviour, so existing tests should probably be unchanged.
================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:23
StringRef Name;
- uint8_t Type;
+ int16_t TypeOrSmc;
+ int64_t Index;
----------------
DiggerLin wrote:
> daltenty wrote:
> > I think using a union or inheritance is probably better than dual purposing this field.
> I am prefer to keep current implement , in the binaryformat/ELF.h , there is no enum type for // Symbol types.
> enum {
> STT_NOTYPE = 0, // Symbol's type is not specified
> STT_OBJECT = 1, // Symbol is a data object (variable, array, etc.)
> STT_FUNC = 2, // Symbol is executable code (function, etc.)
> STT_SECTION = 3, // Symbol refers to a section
> STT_FILE = 4, // Local, absolute symbol that refers to a file
> STT_COMMON = 5, // An uninitialized common block
> STT_TLS = 6, // Thread local data object
> STT_GNU_IFUNC = 10, // GNU indirect function
> STT_LOOS = 10, // Lowest operating system-specific symbol type
> STT_HIOS = 12, // Highest operating system-specific symbol type
> STT_LOPROC = 13, // Lowest processor-specific symbol type
> STT_HIPROC = 15, // Highest processor-specific symbol type
>
> // AMDGPU symbol types
> STT_AMDGPU_HSA_KERNEL = 10
> };
>
> as we discuss offline , if we use the union for the this field, we need to modify the
> ELF.h and add enum name first. I do not want this patch to go further.
>
> I can add comment here in the source code.
> int16_t TypeOrSmc; ///< For Elf, it stores the symbol type,
> ///< for XCOFF, it store the storage mapping class if there is, other wise it store -1.
>
> even if I use the union here for example.
> union {
> ELFType Type;
> StorageMappingClass Smc;
> }
>
> how can I use the union , for example
> AllSymbols[*SecI].emplace_back( .....) ;
>
> or I need the code like
> if(obj->isXCOFF())
> AllSymbols[*SecI].emplace_back( .... )
> else if (obj->isELF())
> AllSymbols[*SecI].emplace_back(..... )
>
>
>
I'm really not happy with `Type` being changed to something else entirely for ELF (and other non-ELF targets for that matter, if any are using it). Not only does the name imply something that is irrelevant for other formats, but you've also changed the type of `Type` to accommodate your desired interface (i.e. -1 being special).
Rather than -1 being special, have you considered using llvm::Optional? If unset, it is -1. Furthermore, why not use inheritance here, if it's viable, or simply adding an extra field? You could even use the union as originally suggested, by simply having two different constructors, one for XCOFF and one for ELF. This would resolve the emplace_back etc issues.
================
Comment at: llvm/include/llvm/MC/MCDisassembler/MCDisassembler.h:23-25
+ int16_t TypeOrSmc; ///< For Elf, it stores the symbol type.
+ ///< for XCOFF, it store storage mapping class of
+ ///< symbol if there is,other wise it store -1.
----------------
hubert.reinterpretcast wrote:
> In English text, this should be "ELF" in all caps.
Lots of typos/bad grammar here, but also see below - I don't think this is the correct thing to do.
For Elf, it stores -> For ELF, this field stores
for -> For
store storage -> stores the storage.
of symbol -> of the symbol
if there is,other wise -> if there is one, otherwise
store -1 -> stores -1
Also, isn't normal style to put the comment above the field name, i.e:
```
/// The comment goes here.
int16_t TypeOrSmc;
```
Finally, your indentation is completely off here. I don't know why, but this certainly isn't what clang-format would give me.
================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1213-1216
+ auto Result = std::find_if(
+ Symbols->begin(), Symbols->end(), [Value](const SymbolInfoTy &Val) {
+ return Val.Addr == static_cast<uint64_t>(Value) &&
+ Val.TypeOrSmc == ELF::STT_NOTYPE;
----------------
Please keep unrelated formatting changes to a separate patch.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:1
+#include "llvm-objdump.h"
+#include "llvm/Object/XCOFFObjectFile.h"
----------------
Missing licence header.
================
Comment at: llvm/tools/llvm-objdump/XCOFFDump.cpp:4
+
+#define SymDoNotHasSmc -1
+
----------------
Named constant rather than macro?
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1128
+static void printIndexNameSmcAsLable(SectionSymbolsTy &Symbols,
+ std::string SymbolName, unsigned int SI) {
----------------
DiggerLin wrote:
> jasonliu wrote:
> > hubert.reinterpretcast wrote:
> > > jasonliu wrote:
> > > > 1. Label mis-spelled in the function name. And we are not necessarily printing the "label", so the name is misleading. I would suggest to name it as printFullyQualifiedSymbolName. Or, printXCOFFSymbolName.
> > > > 2. This function looks very XCOFF centric, do we want to put it in XCOFFDump.cpp?
> > > > 3. We only want one symbol inside of SectionSymbolsTy, so it does not make much sense to pass in a pointer to all of the symbols inside of the section, and a index to just get one symbol. I would try to typedef std::tuple<uint64_t, StringRef, uint8_t, int64_t> somewhere in the header and use that type here. And that should be the only one parameter you need. We don't need to pass in the SymbolName as well.
> > > >
> > > `printXCOFFDecoratedSymbolDescription` or `printXCOFFSymbolDescription`?
> > I'm good with either.
> if (Demangle)
> SymbolName = demangle(SymbolName);
>
> SymbolName maybe demangle , we need to pass SymbolName here.
This function still belongs in the XCOFF file.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1383
SectionAddr + Start + VMAAdjustment);
-
- outs() << SymbolName << ":\n";
+ if (Obj->isXCOFF()) {
+ printXCOFFSymbolDescription(Symbols[SI], SymbolName);
----------------
jasonliu wrote:
> hubert.reinterpretcast wrote:
> > DiggerLin wrote:
> > > daltenty wrote:
> > > > DiggerLin wrote:
> > > > > daltenty wrote:
> > > > > > Shouldn't this behaviour be option gated under an XCOFF specific option? As is this is going to result in output incompatible with binutils objdump on AIX, which could break tools.
> > > > > when dissemble the xcoff object file, we change the label to symbol index+ symbol name + storage mapping class, it is not a new option. I think it is different with the with binutils objdump on AIX.
> > > > The fact that it is different with the with binutils objdump on AIX is the issue. As the top of this file indicates: "The flags and output of this program should be near identical to those of binutils objdump."
> > > >
> > > > We need to add an XCOFF specific option, something like "--expanded-symbol-description", to turn on this behavior change.
> > > @hubert.reinterpretcast , @jasonliu , what do you think about david's opinion ? I am prefer to keep current implement.
> > I think there may be practical issues if we don't follow the binutils behaviour. At the least, it means that having the richer output format by default creates yet another situation where assumptions that people can make on other platforms would not hold for AIX.
> We discussed about it in scrum. And I believe we agreed to the change that David is suggesting.
> But I do hope we have a shorter name or an alias for it though. As we are going to use it in all our test cases and we dont want to type that long option everytime.
> But I do hope we have a shorter name or an alias for it though. As we are going to use it in all our test cases and we dont want to type that long option everytime.
I already commented on the alias point elsewhere. As most users of this are not going to be XCOFF users, it seems like making it clear that this option does nothing for a majority of users is necessary. If this is a very XCOFF-centric piece of functionality, my inclination would be to name the option --xcoff-..., although if the option might conceivably have an implication for other formats too, I'm okay with it not having that leading bit.
================
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));
----------------
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.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:152
+static cl::alias SymbolDescriptionShort(
+ "E", cl::desc("Alias for --Add Symbol Description for dissembly"),
+ cl::NotHidden, cl::Grouping, cl::aliasopt(SymbolDescription));
----------------
hubert.reinterpretcast wrote:
> I would not recommend adding a short option without coordinating the short option with the GNU binutils beforehand.
+1 - we've had to remove short-letter aliases that people gratuitously added in the past from llvm-readelf, so let's not make the same mistake in llvm-objdump.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:910
+#define NO_SYMINDEX -1
+
----------------
I personally think a named constant (i.e. `static const int64_t no_sym_index = -1;`) would be preferable to a macro.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:913-914
+static int64_t getSymbolIndex(const ObjectFile *Obj, const SymbolRef &Sym) {
+ if (!Obj->isXCOFF())
+ return NO_SYMINDEX;
+
----------------
This being XCOFF specific doesn't make sense to me on first glance. ELF symbols have an index too, for example. Either this code needs making generic, or a big TODO/comment needs adding saying something like "implement for other formats".
Alternatively, move this and the next function into the XCOFF file.
================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:932
+ assert(SymRef.getNumberOfAuxEntries() &&
+ "No CSECT Auxiliary Entry is found.");
+
----------------
Spurious double space in assert message.
================
Comment at: llvm/tools/llvm-readobj/XCOFFDumper.cpp:209
Obj.getSymbolIndex(reinterpret_cast<uintptr_t>(AuxEntPtr)));
- if ((AuxEntPtr->SymbolAlignmentAndType & SymbolTypeMask) == XCOFF::XTY_LD)
+ if (AuxEntPtr->isLabel())
W.printNumber("ContainingCsectSymbolIndex", AuxEntPtr->SectionOrLength);
----------------
The changes in this file seem unrelated?
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