[AIX][XCOFF] Fix XCOFFObjectWriter assertion failure with alignment-related gap and improve text section output testing

[DiggerLin]

Summary

1. if there is a gap between the end virtual address of one section and the beginning virtual address of the next section, the XCOFFObjectWriter.cpp will hit a assert.

2.as discussed in the patch https://reviews.llvm.org/D66969,
since implemented the function description. We can output the raw object data for function.
we need to create a test for raw text section content and test section header for xcoff object file.

[daltenty]

llvm/test/CodeGen/PowerPC/aix-return55.ll

21

Should we be checking the function descriptors in this patch as well, since the description of this change indicates we are interested in testing the raw data of the function (and this was pending on the implementation of function descriptors).

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-return55.ll

40

One of the aspects of the code added, which is still in need to testing, is the possible adjustment of the virtual address such that there is a gap between the end virtual address of one section and the beginning virtual address of the next. This test does not exhibit that.

[jasonliu]

Ping on author. Any plan to move this forward? Should it sit in "Plan Changes" queue.

[daltenty]

Minor nit, but otherwise LGTM

llvm/test/CodeGen/PowerPC/aix-return55.ll

5–8

nit: We might want to add a RUN: not case for the 64-bit so we don't forget to add it in the future

22

nit: Remove the disassembly column from the check lines for this and .data, since we are actually only interested in the raw data

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-return55.ll

40

The size of the first section is 0x18 starting at address 0x0. The virtual address of the following section is 0x18. I am not seeing how the virtual address adjustment is demonstrated here.

[DiggerLin]

fixed a bug of llvm/lib/MC/XCOFFObjectWriter.cpp which will hit assert

[DiggerLin]

llvm/test/CodeGen/PowerPC/aix-return55.ll

22

if there is "Disassembly of section .data", it is more clear that the following raw data is data section.

[hubert.reinterpretcast]

llvm/test/CodeGen/PowerPC/aix-return55.ll

40

Thanks. My concern here is addressed; however, the patch has changed scope since the previous approval by @daltenty, so new approvals are needed. I also suggest to make the title of this patch describe the bug fix more than the test.

[hubert.reinterpretcast]

Hi Digger, I would suggest being more descriptive in the title regarding the nature of the bug:
[AIX] Fix XCOFFObjectWriter assertion failure with alignment-related gap; improve text section output testing

[DiggerLin]

In D71845#1896530, @hubert.reinterpretcast wrote:

Hi Digger, I would suggest being more descriptive in the title regarding the nature of the bug:
[AIX] Fix XCOFFObjectWriter assertion failure with alignment-related gap; improve text section output testing

thanks

[jasonliu]

llvm/lib/MC/XCOFFObjectWriter.cpp

440

Instead of removing the assert altogether, I would propose we check after alignment adjustment the CurrentAddressLocation would equal to Section address.
This would help us to detect if we have garbage Section->address value.

And also add a comment similar to this:
There could be gap (but no zero paddings) between each sections.

llvm/test/CodeGen/PowerPC/aix-return55.ll

5–8

Please address this. I'm worried that you have forgotten this because it seems you updated the patch, but this is still not addressed.

23

This line and the next line is not aligned.

[DiggerLin]

address comment

[hubert.reinterpretcast]

llvm/lib/MC/XCOFFObjectWriter.cpp

440

Suggestion:
There could be a gap (without corresponding zero padding) between sections.

442

I think the assert should check that this value is equal to Section->Address without updating CurrentAddessLocation. CurrentAddressLocation can then be updated to Section->Address after the assert.

[DiggerLin]

llvm/lib/MC/XCOFFObjectWriter.cpp

440

after I discussed with Jason offline , and we decide to delete the assertion finally

llvm/test/CodeGen/PowerPC/aix-return55.ll

5–8

these are caused by llvm-objdump -D

[jasonliu]

LGTM except the minor nit.

llvm/lib/MC/XCOFFObjectWriter.cpp

440

Just to clarify, I would still like to have an assertion. But Digger pointed out writing such assertion is non-trivial and the logic is complicated which might not fit in one assert statement.
I don't want to block this patch based on missing one assertion. But if other people feels strong about it and want to suggest a way to write the assertion, please indicate.

Nit: where is the new comment??

llvm/test/CodeGen/PowerPC/aix-return55.ll

5–8

You are replying to the wrong thread.

[hubert.reinterpretcast]

llvm/lib/MC/XCOFFObjectWriter.cpp

440

The major benefit of the assertion can be realized by merely asserting that CurrentAddressLocation is no greater than Section->Address.

[DiggerLin]

address comment

[hubert.reinterpretcast]

LGTM. My understanding is that the symbol table and relocations is already tested elsewhere.

llvm/lib/MC/XCOFFObjectWriter.cpp

 void XCOFFObjectWriter::writeSections(const MCAssembler &Asm,
                                       const MCAsmLayout &Layout) {
   uint32_t CurrentAddressLocation = 0;
   for (const auto *Section : Sections) {
     // Nothing to write for this Section.
     if (Section->Index == Section::UninitializedIndex || Section->IsVirtual)
       continue;
 
-    assert(CurrentAddressLocation == Section->Address &&
-           "Sections should be written consecutively.");
+    CurrentAddressLocation = Section->Address;
     for (const auto *Group : Section->Groups) {
       for (const auto &Csect : *Group) {
         if (uint32_t PaddingSize = Csect.Address - CurrentAddressLocation)
           W.OS.write_zeros(PaddingSize);
         if (Csect.Size)
           Asm.writeSectionData(W.OS, Csect.MCCsect, Layout);
         CurrentAddressLocation = Csect.Address + Csect.Size;
       }
     }
 

llvm/test/CodeGen/PowerPC/aix-return55.ll

-; RUN: llc -mcpu=pwr9 -mtriple=powerpc-ibm-aix-xcoff -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -mcpu=pwr4 -mtriple=powerpc-ibm-aix-xcoff -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -mcpu=pwr4 -mtriple=powerpc-ibm-aix-xcoff -verify-machineinstrs -filetype=obj -o %t.o < %s
+; RUN: llvm-objdump -D %t.o | FileCheck --check-prefix=CHECKOBJ %s
+; RUN: llvm-readobj -sections %t.o | FileCheck --check-prefix=CHECKSECT %s
 
+@a = global i64 320255973571806, align 8
+@d = global double 5.000000e+00, align 8
+@strA = private unnamed_addr constant [12 x i8] c"helloworld\0A\00", align 1
 
 define dso_local signext i32 @foo() {
 entry:
   ret i32 55
 ; CHECK-LABEL: .foo:
 ; CHECK: li 3, 55
 ; CHECK: blr
 }
 
+;CHECKOBJ:      00000000 .text:
+;CHECKOBJ-NEXT:       0: 38 60 00 37                    li 3, 55
+;CHECKOBJ-NEXT:       4: 4e 80 00 20                    blr{{[[:space:]] *}}
+;CHECKOBJ-NEXT: 00000008 .rodata.str1.1:
+;CHECKOBJ-NEXT:       8: 68 65 6c 6c                   xori 5, 3, 27756
+;CHECKOBJ-NEXT:       c: 6f 77 6f 72 xoris 23, 27, 28530 
+;CHECKOBJ-NEXT:       10: 6c 64 0a 00 xoris 4, 3, 2560{{[[:space:]] *}} 
+;CHECKOBJ-NEXT: Disassembly of section .data:{{[[:space:]] *}}
+;CHECKOBJ-NEXT: 00000018 a:
+;CHECKOBJ-NEXT:      18: 00 01 23 45                   <unknown>
+;CHECKOBJ-NEXT:      1c: 67 8a bc de                   oris 10, 28, 48350{{[[:space:]] *}}
+;CHECKOBJ-NEXT: 00000020 d:
+;CHECKOBJ-NEXT:      20: 40 14 00 00                   bdnzf   20, .+0
+;CHECKOBJ-NEXT:      24: 00 00 00 00                   <unknown>{{[[:space:]] *}}
+;CHECKOBJ-NEXT: 00000028 foo:
+;CHECKOBJ-NEXT:      28: 00 00 00 00                   <unknown>
+;CHECKOBJ-NEXT:      2c: 00 00 00 34                   <unknown>
+;CHECKOBJ-NEXT:      30: 00 00 00 00                   <unknown>
+
+;CHECKSECT: Sections [
+;CHECKSECT-NEXT:   Section {
+;CHECKSECT-NEXT:     Index: 1
+;CHECKSECT-NEXT:     Name: .text
+;CHECKSECT-NEXT:     PhysicalAddress: 0x0
+;CHECKSECT-NEXT:     VirtualAddress: 0x0
+;CHECKSECT-NEXT:     Size: 0x14
+;CHECKSECT-NEXT:     RawDataOffset: 0x64
+;CHECKSECT-NEXT:     RelocationPointer: 0x0
+;CHECKSECT-NEXT:     LineNumberPointer: 0x0
+;CHECKSECT-NEXT:     NumberOfRelocations: 0
+;CHECKSECT-NEXT:     NumberOfLineNumbers: 0
+;CHECKSECT-NEXT:     Type: STYP_TEXT (0x20)
+;CHECKSECT-NEXT:   }
+;CHECKSECT-NEXT:   Section {
+;CHECKSECT-NEXT:     Index: 2
+;CHECKSECT-NEXT:     Name: .data
+;CHECKSECT-NEXT:     PhysicalAddress: 0x18
+;CHECKSECT-NEXT:     VirtualAddress: 0x18
+;CHECKSECT-NEXT:     Size: 0x1C
+;CHECKSECT-NEXT:     RawDataOffset: 0x78
+;CHECKSECT-NEXT:     RelocationPointer: 0x94
+;CHECKSECT-NEXT:     LineNumberPointer: 0x0
+;CHECKSECT-NEXT:     NumberOfRelocations: 2
+;CHECKSECT-NEXT:     NumberOfLineNumbers: 0
+;CHECKSECT-NEXT:     Type: STYP_DATA (0x40)
+;CHECKSECT-NEXT:   }
+;CHECKSECT-NEXT: ]