[PATCH] D66969: Output XCOFF object text section

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:328
+  if (putNameInStringTable(SymbolName)) {
+    W.write<uint32_t>(0);
+    W.write<uint32_t>(Strings.getOffset(SymbolName));
----------------
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.


================
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?
----------------
Neither GCC nor XL fail to set the function property when debugging is enabled. This is at least a 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);
----------------
s/symbol/symbol's/;


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:357
+  // Now output the auxiliary entry.
+  W.write<uint32_t>(CSectionRef.SymbolTableIndex);
+  // Parameter typecheck hash. Not supported.
----------------
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`.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:363
+  // Symbol type: Label
+  W.write<int8_t>(XCOFF::XTY_LD);
+  // Storage mapping class.
----------------
This field is unsigned; see D65159:
```
W.write<uint8_t>(getEncodedType(Sec.MCCsect));
```


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:365
+  // Storage mapping class.
+  W.write<int8_t>(CSectionRef.MCCsect->getMappingClass());
+  // Reserved (x_stab).
----------------
Ditto.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:367
+  // Reserved (x_stab).
+  W.write<int32_t>(0);
+  // Reserved (x_snstab).
----------------
Ditto.


================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:369
+  // Reserved (x_snstab).
+  W.write<int16_t>(0);
+}
----------------
Ditto.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969