[PATCH] D66969: Output XCOFF object text section
Digger via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 17 07:44:32 PDT 2019
DiggerLin marked 23 inline comments as done.
DiggerLin added inline comments.
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:53
struct Symbol {
- const MCSymbolXCOFF *const MCSym;
+ const MCSymbolXCOFF *MCSym;
uint32_t SymbolTableIndex;
----------------
hubert.reinterpretcast wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > What prompted the removal of the `const`?
> > if define as const MCSymbolXCOFF *const MCSym; , it can not have a default assignment operator for the struct Symbol. a default assignment operator will be need in the SmallVector<Symbol, 1> Syms; it will have compile error .
> Is this answer in relation to some specific use of `Syms` or its containing class?
the problem is happened in the // Print out symbol table for the data section.
// Print out symbol table for the program code.
for (const auto CSection : ProgramCodeCsects) {
// Write out the control section first and then each symbol in it.
writeSymbolTableEntryForControlSection(CSection, Text.Index,
CSection.MCCsect->getStorageClass());
for (const auto Sym : CSection.Syms) {
writeSymbolTableEntryForCsectMemberLabel(Sym, CSection, Text.Index,
Layout.getSymbolOffset(*(Sym.MCSym)));
}
}
for (const auto CSection : ProgramCodeCsects) {
it will caused Symbol::operator=. since we will change to for (const auto& CSection : ProgramCodeCsects) in new copy, I will recover "const MCSymbolXCOFF *const MCSym;"
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:153
+ void writeSymbolTableEntryForSymbol(const Symbol &, const ControlSection &,
+ const int16_t, uint64_t);
+ void writeSymbolTableEntryForControlSection(const ControlSection &,
----------------
hubert.reinterpretcast wrote:
> Stray top-level `const` on function parameter declaration on a function declaration.
change the "const int16_t" to "int16_t"
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:155
+ void writeSymbolTableEntryForControlSection(const ControlSection &,
+ const int16_t,
+ XCOFF::StorageClass);
----------------
hubert.reinterpretcast wrote:
> Stray top-level `const` on function parameter declaration on a function declaration.
change the "const int16_t" to "int16_t"
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:293
+ // Write the program code control sections one at a time.
+ for (const auto CSection : ProgramCodeCsects)
+ Asm.writeSectionData(W.OS, CSection.MCCsect, Layout);
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > Please don't copy the `ControlSection`s.
> We already use `Sec` and `Csect` for similar cases. I don't think we need to add `CSection` into the mix.
change as suggestion
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:298
+uint64_t XCOFFObjectWriter::writeObject(MCAssembler &Asm,
+ const llvm::MCAsmLayout &Layout) {
// We always emit a timestamp of 0 for reproducibility, so ensure incremental
----------------
hubert.reinterpretcast wrote:
> Is the `llvm::` qualification necessary?
yes, it is not necessary, the file already has using namespace llvm;
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:322
+bool XCOFFObjectWriter::putNameInStringTable(const StringRef &SymbolName) {
+ return SymbolName.size() >= XCOFF::NameSize;
----------------
hubert.reinterpretcast wrote:
> `putNameInStringTable` sounds like an action. Maybe use `nameShouldBeInStringTable`.
I prefer to use isNameInStringTable
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:323
+bool XCOFFObjectWriter::putNameInStringTable(const StringRef &SymbolName) {
+ return SymbolName.size() >= XCOFF::NameSize;
+}
----------------
hubert.reinterpretcast wrote:
> Names of `NameSize` length are placed in the symbol table.
>
> Please add appropriate testing. This is a regression that was not caught.
changed, and I have asked Xing to add some test case for symbol name which length is 8 and symbol name which length large than 8 . @xingxue
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:326
+
+void XCOFFObjectWriter::writeSymbolName(const StringRef &SymbolName) {
+ if (putNameInStringTable(SymbolName)) {
----------------
hubert.reinterpretcast wrote:
> This function is misnamed for what it does. It writes the symbol table entry name field, not the symbol name.
it write symbol name information in the function, I think writeSymbolName is OK.
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:328
+ if (putNameInStringTable(SymbolName)) {
+ W.write<uint32_t>(0);
+ W.write<uint32_t>(Strings.getOffset(SymbolName));
----------------
hubert.reinterpretcast wrote:
> Minor nit: This was fixed in D65159 to use `int32_t` (not that there is a real difference). However this is my second comment on this patch regarding its replacement of more correct code with less correct code.
changed.thanks
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:349
+ // visibility, and all other bits are either optionally set or reserved,
+ // this is always zero.
+ // TODO FIXME How to assert a symbols visibilty is default?
----------------
hubert.reinterpretcast wrote:
> Neither GCC nor XL fail to set the function property when debugging is enabled. This is at least a TODO.
add a new TODO
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:350
+ // this is always zero.
+ // TODO FIXME How to assert a symbols visibilty is default?
+ W.write<uint16_t>(0);
----------------
hubert.reinterpretcast wrote:
> s/symbol/symbol's/;
done
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:365
+ // Storage mapping class.
+ W.write<int8_t>(CSectionRef.MCCsect->getMappingClass());
+ // Reserved (x_stab).
----------------
hubert.reinterpretcast wrote:
> Ditto.
changed.
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:369
+ // Reserved (x_snstab).
+ W.write<int16_t>(0);
+}
----------------
hubert.reinterpretcast wrote:
> Ditto.
changed
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:373
+void XCOFFObjectWriter::writeSymbolTableEntryForControlSection(
+ const ControlSection &CSectionRef, const int16_t SectionIndex,
+ XCOFF::StorageClass StorageClass) {
----------------
hubert.reinterpretcast wrote:
> Ditto re: top-level `const`.
fixed , thanks
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:382
+ // n_type
+ W.write<uint16_t>(0);
+ // n_sclass
----------------
hubert.reinterpretcast wrote:
> Please add a comment about this value being always zero (for now). Again, this was better in the existing code before this patch.
added a comment
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:395
+ // Symbol type.
+ W.write<int8_t>(getEncodedType(CSectionRef.MCCsect));
+ // Storage mapping class.
----------------
hubert.reinterpretcast wrote:
> Unsigned.
changed.thanks
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:399
+ // Reserved (x_stab).
+ W.write<int32_t>(0);
+ // Reserved (x_snstab).
----------------
hubert.reinterpretcast wrote:
> Unsigned.
changed.thanks
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:401
+ // Reserved (x_snstab).
+ W.write<int16_t>(0);
+}
----------------
hubert.reinterpretcast wrote:
> Unsigned.
changed.thanks
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:448
-void XCOFFObjectWriter::writeSymbolTable() {
- assert(ProgramCodeCsects.size() == 0 && ".text csects not handled yet.");
+void XCOFFObjectWriter::writeSymbolTable(const llvm::MCAsmLayout &Layout) {
+ // Print out symbol table for the program code.
----------------
hubert.reinterpretcast wrote:
> Same question re: `llvm::` as before.
deleted
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:450
+ // Print out symbol table for the program code.
+ for (const auto CSection : ProgramCodeCsects) {
+ // Write out the control section first and then each symbol in it.
----------------
hubert.reinterpretcast wrote:
> Please don't copy the `ControlSection`s.
changed
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:488
+ Text.Index = SectionIndex++;
+ assert(Text.Index <= INT16_MAX && "Text section index overflow!");
+ uint32_t StartAddress = Address;
----------------
hubert.reinterpretcast wrote:
> Technically this assert is too late. `INT_MAX` is allowed to be the same as `INT16_MAX`, so the addition may overflow even in the promoted type.
I delete the assert, I do not think we need assert here.
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D66969/new/
https://reviews.llvm.org/D66969
More information about the llvm-commits
mailing list