[PATCH] D61532: implement of the parsing symbol table for xcoffobjfile and output as yaml format
Digger via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed May 15 11:49:38 PDT 2019
DiggerLin added inline comments.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:57
+static StringRef generateStringRef(const char *Name, uint64_t size) {
+ auto NulCharPtr = static_cast<const char *>(memchr(Name, '\0', size));
----------------
hubert.reinterpretcast wrote:
> Use `Size` instead of `size` to match `Name` and also the corresponding parameter in `checkSize`.
fixed , thanks
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:108
+
+ if (SymEntPtr->NameInStrTbl.Magic != 0)
+ return generateStringRef(SymEntPtr->SymbolName, XCOFF::SymbolNameSize);
----------------
hubert.reinterpretcast wrote:
> Hardcoded `0` should be replaced with something that indicates the semantic: `XCOFFSymbolEntry::NAME_IN_STR_TBL_MAGIC` or at least `0x00000000`.
changed as suggestion
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:112
+ if (SymEntPtr->SectionNumber == XCOFF::N_DEBUG) {
+ llvm_unreachable("Not yet implemented!");
+ return StringRef(nullptr, 0);
----------------
hubert.reinterpretcast wrote:
> If an error recovery path is thought to be necessary, then `llvm_unreachable` is the wrong answer. Since the error recovery seems reasonable, use `assert` instead.
we do not support xcoff object file which generated by -g currently. we will support it later, and symbol name in the .debug section later. So I think use llvm_unreachable maybe more reasonable.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:119
+ // or .debug section.
+ // A byte offset value less than 4 is a null or zero-length symbol name.
+ if (Offset < 4)
----------------
hubert.reinterpretcast wrote:
> > A byte offset value of 0 is a null or zero-length symbol name.
> Is the "less than 4" an invention of this patch? A byte offset of 1, 2, or 3 points into the length field. It is well-formed as claimed by the comment in the patch, or is it erroneous?
in the xcoff document, it describe as " A byte offset value of 0 is a null or zero-length symbol name."
A byte offset of 1, 2, or 3 points into the length field , the document do not talk about anything on it. we look it as "zero-length symbol name" . If we can treated the "A byte offset of 1, 2, or 3" as error format, and return as
make_error<GenericBinaryError>("Symbol Name offset error",object_error::parse_failed);
But I think looking "A byte offset of 1, 2, or 3" as "zero-length symbol name" will make obj2yaml more compatible。
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:124
+ if (StringTable.Data != nullptr)
+ return (StringTable.Data + Offset);
+
----------------
hubert.reinterpretcast wrote:
> Check that the `Offset` is within the string table.
Good idea, added offset check with the string table size. thanks
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:126
+
+ return make_error<GenericBinaryError>("Symbol Name parse failed.",
+ object_error::parse_failed);
----------------
hubert.reinterpretcast wrote:
> It seems that there should be no period in the string:
> ```
> llvm/lib/Object/WasmObjectFile.cpp: make_error<StringError>("Bad magic number", object_error::parse_failed);
> ```
deleted the period.
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:443
+ // field
+ if (!checkSize(Data, EC, CurPtr, (StringTable.Size)))
+ return;
----------------
hubert.reinterpretcast wrote:
> Why the extra parentheses around `StringTable.Size`?
deleted parentheses
================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:100
+ Symb.p = reinterpret_cast<uintptr_t>(SymEntPtr);
return;
}
----------------
hubert.reinterpretcast wrote:
> sfertile wrote:
> > this is unneeded.
> There's still a stray `return` statement here.
deleted
================
Comment at: llvm/lib/ObjectYAML/XCOFFYAML.cpp:20
+#include <cstdint>
+#include <vector>
----------------
hubert.reinterpretcast wrote:
> Both of the above includes are included directly by `XCOFFYAML.h` for which this is the implementation file.
good point, I deleted all redundant header files
================
Comment at: llvm/test/tools/obj2yaml/aix_xcoff.test:17
+# CHECK-NEXT: Section: N_DEBUG
+# CHECK-NEXT: Type: 0x0003
+# CHECK-NEXT: StorageClass: C_FILE
----------------
hubert.reinterpretcast wrote:
> The question for this line is whether printing as a single type field is appropriate. This is a `C_FILE` entry where `n_lang` is `C (0x00)` and `n_cputype` is `COM (0x03)`. That is, perhaps we should have inspected `CFileLanguageIdAndTypeId`?
The field name of yaml output are defined in the function, Once it is defined , we can not change the field name based on the value.
void MappingTraits<XCOFFYAML::Symbol>::mapping(IO &IO, XCOFFYAML::Symbol &S) {
IO.mapRequired("Name", S.SymbolName);
IO.mapRequired("Value", S.Value);
IO.mapRequired("Section", S.SectionName);
IO.mapRequired("Type", S.Type);
IO.mapRequired("StorageClass", S.StorageClass);
IO.mapRequired("NumberOfAuxEntries", S.NumberOfAuxEntries);
}
================
Comment at: llvm/test/tools/obj2yaml/aix_xcoff.test:17
+# CHECK-NEXT: Section: N_DEBUG
+# CHECK-NEXT: Type: 0x0003
+# CHECK-NEXT: StorageClass: C_FILE
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > DiggerLin wrote:
> > > sfertile wrote:
> > > > Is the intent to always keep dumping the type as a numeric value? For this tool I think that's reasonable but I'd like to clarify.
> > > for the N_ABS, N_DEBUG, N_UNDEF, which do not has section related to it, It not show section number here.
> > Thanks for Hubert let me I make a wrong answer.
> >
> > 1. for the question of " Is the intent to always keep dumping the type as a numeric value? For this tool I think that's reasonable but I'd like to clarify."
> >
> > Answer:
> > in the xcoff document ,
> > n_type :
> > Bits 0-3
> > Symbol visibility. The SYM_V_MASK macro, with the value 0xF000, can be used to mask off bits in the n_type field that do not specify visibility. The following visibilities are defined:
> > 0x1000 SYM_V_INTERNAL
> > 0x2000 SYM_V_HIDDEN
> > 0x3000 SYM_V_PROTECTED
> > 0x4000 SYM_V_EXPORTED
> >
> > Bit 10
> > Optionally set to 1 if the symbol is a function. Otherwise, it is set to 0.
> >
> > I can not use enum to represent the value. I have to display as a value.
> >
> > 2. For the question "This is a C_FILE entry where n_lang is C (0x00) and n_cputype is COM (0x03). That is, perhaps we should have inspected CFileLanguageIdAndTypeId?"
> >
> > Answer: I do not think yaml framework support showing different field name based on the value of the field.
> >
> > The field name of yaml output are defined in the function, Once it is defined , we can not change the field name based on the value.
> >
> > void MappingTraits<XCOFFYAML::Symbol>::mapping(IO &IO, XCOFFYAML::Symbol &S) {
> > IO.mapRequired("Name", S.SymbolName);
> > IO.mapRequired("Value", S.Value);
> > IO.mapRequired("Section", S.SectionName);
> > IO.mapRequired("Type", S.Type);
> > IO.mapRequired("StorageClass", S.StorageClass);
> > IO.mapRequired("NumberOfAuxEntries", S.NumberOfAuxEntries);
> > }
> >
> What happens with something like:
> ```
> llvm/lib/ObjectYAML/COFFYAML.cpp: IO.mapOptional("SymbolTableIndex", Rel.SymbolTableIndex);
> ```
> ?
when you did the first comment on it , I think over to use IO.mapOptional, But I denied it in final, "n_type" is not optional field in the xcoff object file. if I use optional field for the CFileLanguageIdAndTypeId , it means that CFileLanguageIdAndTypeId is optional field of symbol entry , It may cause confuse, it actually only a different interpret of the n_type.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61532/new/
https://reviews.llvm.org/D61532
More information about the llvm-commits
mailing list