[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:488
+    Text.Index = SectionIndex++;
+    assert(Text.Index <= INT16_MAX && "Text section index overflow!");
+    uint32_t StartAddress = Address;
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > Technically this assert is too late. `INT_MAX` is allowed to be the same as `INT16_MAX`, so the addition may overflow even in the promoted type.
> I delete the assert, I do not think we need assert here.
I'm OK with postponing the overflow checking for now, since there is only 3 sections we might be emitting. 


================
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.
----------------
DiggerLin wrote:
> 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"
I don't see how the change helps readability. Originally we have a single variable that represent the next assignable address in the file. When we assign a section start address it simply starts at that next available address. After the change we now have a variable to track the `SectionStartAddress`, which doesn't seem any different or special compared to any of the other addresses we assign. And we have the extra overhead of creating a local and assigning that with the value of section start address, then modifying that in the loop making sure we have to copy its modified value out to `SectionStartAddress` at the end of the loop. Why is the sections start address different from any other address we assign in the loop? In what way does having the extra variable increase readability? 



================
Comment at: llvm/lib/MC/XCOFFObjectWriter.cpp:354
+  // TODO FIXME How to assert a symbol's visibilty is default?
+  // TODO  Set the function indicator (bit 10, 0x0020) for functions
+  // when debugging is enabled.
----------------
Minor nit: Extra space after  `TODO`


================
Comment at: llvm/test/CodeGen/PowerPC/aix-return55.ll:7
+  ret i32 55
+; CHECK-LABEL: foo
+; CHECK: li 3, 55
----------------
DiggerLin wrote:
> hubert.reinterpretcast wrote:
> > `foo:` with the colon. `.foo:` if possible.
> changed .thanks
Still missing the `:` in the CHECK-LABEL line.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D66969