[PATCH] D66969: Output XCOFF object text section

[Digger via Phabricator via llvm-commits]

DiggerLin marked 2 inline comments as done.
DiggerLin added a comment.





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


================
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.
----------------
sfertile wrote:
> 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.
there is a patch https://reviews.llvm.org/D67125 , which will change Address to SectionStartAddress.  And we think SectionStartAddress can express meaning than Address.  

What we need  another available Address in the uint32_t Address = SectionStartAddress; , the Address is used to store the start address of the Csection. 

there is a more variable "Address" as your pointout, but I think the code will more readable with the additional  variable "Address"


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969