[PATCH] D66969: Output XCOFF object text section

[Digger via Phabricator via llvm-commits]

DiggerLin marked 12 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/MC/MCXCOFFStreamer.cpp:78
+
+  // TODO: Handle Fixups Later
+
----------------
jasonliu wrote:
> Minor nit: "Handle Fixups later."
changed as  suggestion.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:230
+    // Add the section name to the string table in case we need it.
+    // At this point we assume that the same rules apply to names for sections
+    // as would for names of symbols (ie >= XCOFF::NameSize).
----------------
sfertile wrote:
> DiggerLin wrote:
> > sfertile wrote:
> > > Why are we 'assuming'? The name length limitation is a property of the symbol table entries, so we know it must use the same rules.
> > changed the comment.
> I think the wording of the comment is a bit awkward. I'd be ok with no comment on this block (the code is kind-of self documenting), otherwise maybe the same comment to the one you added for symbols.Its worded pretty well:
> 
> ```
>   // If the name does not fit in the storage provided in the symbol table
>   // entry, add it to the string table.
> ```
changed as  suggestion


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:53
 struct Symbol {
-  const MCSymbolXCOFF *const MCSym;
+  const MCSymbolXCOFF *MCSym;
   uint32_t SymbolTableIndex;
----------------
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 .


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:71
   SmallVector<Symbol, 1> Syms;
-
+  StringRef getSectionName() const { return MCCsect->getSectionName(); }
   ControlSection(const MCSectionXCOFF *MCSec)
----------------
sfertile wrote:
> minor nit: `getName()` instead to keep it consistent with the symbol wrapper class.
changed as  suggestion


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:92
 
   uint16_t Index;
 
----------------
jasonliu wrote:
> Should this also be int16_t? Since the Index could be negative value it seems. 
good catching.  thanks. I will fix it



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:92
 
   uint16_t Index;
 
----------------
sfertile wrote:
> DiggerLin wrote:
> > jasonliu wrote:
> > > Should this also be int16_t? Since the Index could be negative value it seems. 
> > good catching.  thanks. I will fix it
> > 
> I'll try to answer since I originally committed this wrapper class. I don't //think// we need t be able to represent actual sections with a negative index. For 'real' sections I believe the index will be a number always greater than or equal to 1. The other sections indexes are:
> 
>  0: This will be the section index on symbol table entries for references to symbols defined externally and we don't know what section its in.
> -1: This will be the section index on symbol table entries of absolute symbols. So not in a section.
> -2: Some debug symbols will have this section index. In this case I'm not sure if its similar to  absolute symbols, as in the debug symbols are not contained in a section, or if this is some special debug section that must be created, in which case this filed will have to change type.
> 
> It doesn't hurt to change the field to a signed type anyway. Although I suggest we do it in a separate patch, that also adds overflow checking to when we assign the section indexes.
changed as suggestion.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:341
+    const int16_t Index, uint64_t SymbolOffset) {
+  // n_name, n_zeros, n_offset
+  writeSymbolName(SymbolRef.getName());
----------------
sfertile wrote:
> The comment makes it seem like we are about to write three fields, n_name *and* n_zeros *and* n_offset.  We have a descriptive function name so I don't think we need a comment, but if you want one i think it needs to be clear we are writing either `name` or `zeros and string table offset`.
> 
changed as  suggestion


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:372
+void XCOFFObjectWriter::writeSymbolTableEntryForControlSection(
+    const ControlSection &CSectionRef, const int16_t Index,
+    XCOFF::StorageClass StorageClass) {
----------------
jasonliu wrote:
> Index -> SectionIndex
changed,thanks



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:487
+    Text.Index = SectionIndex++;
+    uint32_t StartAddress = Address;
+    for (auto &Csect : ProgramCodeCsects) {
----------------
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. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969