[BUG-FIX][XCOFF] fixed a bug of XCOFFObjectFile.cpp when there is padding at the last csect of a sections

[DiggerLin]

Summary

fixed a bug of XCOFFObjectFile.cpp when there is padding at the last csect of a sections.
when there is a tail padding of a section, but the value of CurrentAddressLocation do not be increased by the padding size. it will hit assert assert(CurrentAddressLocation == Section->Address && "We should have no padding between sections.");

[daltenty]

I really think this should probably be split, at least definitely for the commits, as there are a few separate issues going on here that don't seem related.

llvm/lib/MC/XCOFFObjectWriter.cpp

366

Why were we previously expecting and enforcing no padding, but now we allow it?

422

Is it possible to use __attribute__ ((nonstring)) instead?

llvm/test/CodeGen/PowerPC/aix-xcoff-mergeablestring.ll

1 ↗

(On Diff #231543)

This should be merged with the existing tests in llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll? Also see the comment there.

[DiggerLin]

llvm/lib/MC/XCOFFObjectWriter.cpp

366

when there is a tail padding of a section, but the value of CurrentAddressLocation do not be increased by the padding size. it will hit assert assert(CurrentAddressLocation == Section->Address && "We should have no padding between sections.");

422

using the attribute will caused some other compiler warning . We can not solve a warning and introduce other warning, Current fix a safety fix, I am prefer to keep it.

llvm/test/CodeGen/PowerPC/aix-xcoff-mergeablestring.ll

1 ↗

(On Diff #231543)

thanks for your suggestion.

[DiggerLin]

change test case based on comment.

[jasonliu]

I agree with David about splitting. There are 4 issues mentioned in this patch, and they are not related. (I'm not sure if 1 or 3 could be combine or not)
Putting them together makes it hard to review, and hard to determine if the test case actually covered the issue that's raised.
Let's make separate patches if possible.

[daltenty]

llvm/lib/MC/XCOFFObjectWriter.cpp

359

This can be merged into the loop body now

366

Combine this with the declaration

[hubert.reinterpretcast]

llvm/lib/MC/XCOFFObjectWriter.cpp

366

The assert has the effect to detecting if we assigned the section addresses in a different order than we are writing the sections. We should update the assert to indicate that "We should be writing sections consecutively."

382–384

We can update CurrentAddressLocation here instead.

[DiggerLin]

llvm/lib/MC/XCOFFObjectWriter.cpp

366

thank for reminding me the problem.

[hubert.reinterpretcast]

LGTM with minor comments.

llvm/lib/MC/XCOFFObjectWriter.cpp

366

Minor nit: We have been using periods to end full sentences in asserts. Add a period here.

llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll

11

Minor nit: We did not use two spaces before the < %s on the lines above. Let's not use it here.

[daltenty]

Other than minor nit, LGTM

llvm/lib/MC/XCOFFObjectWriter.cpp

 void XCOFFObjectWriter::recordRelocation(MCAssembler &, const MCAsmLayout &,
                                          const MCFragment *, const MCFixup &,
                                          MCValue, uint64_t &) {
   // TODO: recordRelocation is not yet implemented.
 }
 
 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 &&
-           "We should have no padding between sections.");
+           "Sections should be written consecutively.");
     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;
       }
     }
 
     // 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.
     if (uint32_t PaddingSize =
-            Section->Address + Section->Size - CurrentAddressLocation)
+            Section->Address + Section->Size - CurrentAddressLocation) {
       W.OS.write_zeros(PaddingSize);
+      CurrentAddressLocation += PaddingSize;
+    }
   }
 }
 
 uint64_t XCOFFObjectWriter::writeObject(MCAssembler &Asm,
                                         const MCAsmLayout &Layout) {
   // We always emit a timestamp of 0 for reproducibility, so ensure incremental
   // linking is not enabled, in case, like with Windows COFF, such a timestamp
   // is incompatible with incremental linking of XCOFF.
bool XCOFFObjectWriter::nameShouldBeInStringTable(const StringRef &SymbolName) {
   return SymbolName.size() > XCOFF::NameSize;
 }
 
 void XCOFFObjectWriter::writeSymbolName(const StringRef &SymbolName) {
   if (nameShouldBeInStringTable(SymbolName)) {
     W.write<int32_t>(0);
     W.write<uint32_t>(Strings.getOffset(SymbolName));
   } else {
     char Name[XCOFF::NameSize+1];
     std::strncpy(Name, SymbolName.data(), XCOFF::NameSize);
     ArrayRef<char> NameRef(Name, XCOFF::NameSize);
     W.write(NameRef);
   }
 }
 
 void XCOFFObjectWriter::writeSymbolTableEntryForCsectMemberLabel(
     const Symbol &SymbolRef, const ControlSection &CSectionRef,

llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll

 ; This file tests the codegen of mergeable strings in AIX assembly only.
 ; Once the codegen of mergeable strings for XCOFF object files is supported
 ; the test in this file should be merged into aix-xcoff-data.ll with additional
 ; tests for XCOFF object files.
 
 ; RUN: llc -verify-machineinstrs -mcpu=pwr7 \
 ; RUN:     -mtriple powerpc-ibm-aix-xcoff < %s | FileCheck %s
 ; RUN: llc -verify-machineinstrs -mcpu=pwr7 \
 ; RUN:     -mtriple powerpc64-ibm-aix-xcoff < %s | FileCheck %s
 
+; RUN: llc -verify-machineinstrs -mcpu=pwr7 -mtriple powerpc-ibm-aix-xcoff -filetype=obj -o %t.o < %s
+; RUN: llvm-objdump -D %t.o | FileCheck --check-prefix=CHECKOBJ %s
+
 @strA = private unnamed_addr constant [14 x i8] c"hello world!\0A\00", align 1
+@.str = private unnamed_addr constant [9 x i8] c"abcdefgh\00", align 1
+@p = global i8* getelementptr inbounds ([9 x i8], [9 x i8]* @.str, i32 0, i32 0), align 4
+
+; Function Attrs: noinline nounwind optnone
+define i8 @foo() #0 {
+entry:
+  %0 = load i8*, i8** @p, align 4
+  %1 = load i8, i8* %0, align 1
+  ret i8 %1
+}
 
 ; CHECK:      .csect .rodata.str1.1[RO]
 ; CHECK-NEXT: .LstrA:
 ; CHECK-NEXT: .byte   104
 ; CHECK-NEXT: .byte   101
 ; CHECK-NEXT: .byte   108
 ; CHECK-NEXT: .byte   108
 ; CHECK-NEXT: .byte   111
 ; CHECK-NEXT: .byte   32
 ; CHECK-NEXT: .byte   119
 ; CHECK-NEXT: .byte   111
 ; CHECK-NEXT: .byte   114
 ; CHECK-NEXT: .byte   108
 ; CHECK-NEXT: .byte   100
 ; CHECK-NEXT: .byte   33
 ; CHECK-NEXT: .byte   10
 ; CHECK-NEXT: .byte   0
+; CHECK-NEXT: .L.str:
+; CHECK-NEXT: .byte   97
+; CHECK-NEXT: .byte   98
+; CHECK-NEXT: .byte   99
+; CHECK-NEXT: .byte   100
+; CHECK-NEXT: .byte   101
+; CHECK-NEXT: .byte   102
+; CHECK-NEXT: .byte   103
+; CHECK-NEXT: .byte   104
+; CHECK-NEXT: .byte   0
+
+; CHECKOBJ: 00000010 .rodata.str1.1:
+; CHECKOBJ-NEXT:       10: 68 65 6c 6c                   xori 5, 3, 27756
+; CHECKOBJ-NEXT:       14: 6f 20 77 6f                   xoris 0, 25, 30575
+; CHECKOBJ-NEXT:       18: 72 6c 64 21                   andi. 12, 19, 25633
+; CHECKOBJ-NEXT:       1c: 0a 00 61 62                   tdlti   0, 24930{{[[:space:]] *}}
+; CHECKOBJ-NEXT: 0000001e .L.str:
+; CHECKOBJ-NEXT:       1e: 61 62 63 64                   ori 2, 11, 25444
+; CHECKOBJ-NEXT:       22: 65 66 67 68                   oris 6, 11, 26472
+; CHECKOBJ-NEXT:       26: 00                            <unknown>
+; CHECKOBJ-NEXT:       27: 00                            <unknown>