[PATCH] D66969: Output XCOFF object text section

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:53
 struct Symbol {
-  const MCSymbolXCOFF *const MCSym;
+  const MCSymbolXCOFF *MCSym;
   uint32_t SymbolTableIndex;
----------------
DiggerLin wrote:
> 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;"
> 
> 
> 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:487
+    Text.Index = SectionIndex++;
+    uint32_t StartAddress = Address;
+    for (auto &Csect : ProgramCodeCsects) {
----------------
DiggerLin wrote:
> sfertile wrote:
> > We should be assigning Text.Address even though that is expected to always be virtual/physical address 0 anyway.
> we assigned  uint32_t Address = 0; at begin, I do not think we need to assert  assigning Text.Address here, because if always true here. 
> we assigned uint32_t Address = 0; at begin, I do not think we need to assert assigning Text.Address here, because if always true here.
I'm suggesting we need to assign `Text.Address = Address` even though we expect `Address` to be zero. If we for example decided there should be a section mapped before the .text section in the object file then `Address`  may not be zero.I understand its unlikely for us to make such a change, but that is not a good reason to skip assigning the address.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:357
+  // Now output the auxiliary entry.
+  W.write<uint32_t>(CSectionRef.SymbolTableIndex);
+  // Parameter typecheck hash. Not supported.
----------------
hubert.reinterpretcast wrote:
> Since the field is named `SectionLen` in `llvm::object::XCOFFCsectAuxEnt32`, a comment is warranted regarding its use also for referencing the containing csect by symbol table index. Please also add a comment in `include/llvm/Object/XCOFFObjectFile.h`.
I'm not disagreeing with this, but it should be done in a separate patch. 


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:356
+  // TODO FIXME How to assert a symbol's visibilty is default?
+  // TODO support Bit10 for a function symbol in the old XCOFF32 interpretation
+  W.write<uint16_t>(0);
----------------
Minor nit: IIUC We aren't supporting the old XCOFF32 interpretation, but rather setting the bit (which is optional in the new interpretation) when debugging is enabled.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:490
   // section header table.
-  uint32_t Address = 0;
+  uint32_t SectionStartAddress = 0;
   // Section indices are 1-based in XCOFF.
----------------
Why did you change this, then add an 'Address' in each of the section layout calculations? We can use `Text.Address`/`BSS.Address` to calculate the sections size, so no need for an extra variable.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66969/new/

https://reviews.llvm.org/D66969