[PATCH] D66969: Output XCOFF object text section header and symbol entry for program code
Hubert Tong via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 9 14:22:11 PDT 2019
hubert.reinterpretcast added inline comments.
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:518
+ Csect.Size = Layout.getSectionAddressSize(MCSec);
+ Address = Csect.Address + Csect.Size;
+ Csect.SymbolTableIndex = SymbolTableIndex;
----------------
There's two spaces after the `+`.
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:296
+ // Write the program code control sections one at a time.
+ uint32_t PreCSectEndAddress = Text.Address;
+ uint32_t PaddingSize;
----------------
Suggestion: `CurrentAddressLocation`
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:299
+ for (const auto &Csect : ProgramCodeCsects) {
+ // PaddingSize = Virtual address of current CSect - Virtual end address of
+ // previous CSect.
----------------
I think the code can be made sufficiently self-explanatory that we don't need a comment here.
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:302
+ PaddingSize = Csect.Address - PreCSectEndAddress;
+ if (PaddingSize)
+ W.OS.write_zeros(PaddingSize);
----------------
The above write to `PaddingSize` is only read by the `if` and the use inside the `if`.
```
if (uint32_t PaddingSize = Csect.Address - CurrentAddressLocation)
```
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:308
+
+ // Padding Size of Tail Section =
+ // Virtual end address of current Section - Virtual end address of last CSect.
----------------
Suggestion:
The size of the tail padding in a section is the end virtual address of the current section minus the the end virtual address of the last csect in that section.
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:310
+ // Virtual end address of current Section - Virtual end address of last CSect.
+ if (ProgramCodeCsects.size()) {
+ PaddingSize = Text.Address + Text.Size - PreCSectEndAddress;
----------------
```
if (!ProgramCodeCsects.empty())
```
however, I suggest checking the section and not the group of csects (they aren't the same thing):
```
if (Text.Index != -1)
```
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:312
+ PaddingSize = Text.Address + Text.Size - PreCSectEndAddress;
+ if (PaddingSize)
+ W.OS.write_zeros(PaddingSize);
----------------
Same comment about the write to `PaddingSize`.
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:531
+
+ // First Csect of each section do not need padding zero. We need to
+ // adjust section virtual address to first Csect's address.
----------------
Use "csect" instead of "Csect" when using the term in an English context where the word would not be capitalized.
Suggestion:
The first csect of a section can be aligned by adjusting the virtual address of its containing section instead of writing zeroes into the object file.
================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:544
BSS.Index = SectionIndex++;
- assert(alignTo(Address, DefaultSectionAlign) == Address &&
- "Improperly aligned address for section.");
- uint32_t StartAddress = Address;
+ // We use alignment address of previous section as BSS start address.
+ BSS.Address = Address;
----------------
The difference in the calculation for the virtual address of the `.bss` section and that of the `.text` section might complicate efforts to common up the handling. Note that a change in how the virtual address of `.bss` is calculated is within the scope of this patch because it changes the value from being always zero.
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