Page MenuHomePhabricator

[XCOFF][AIX] report_fatal_error when an overflow section is needed
ClosedPublic

Authored by jasonliu on Jun 3 2020, 8:32 AM.

Details

Summary

If there are more than 65534 relocation entries in a single section, we should generate an overflow section.
Since we don't support overflow section for now, we should generate an error.

Diff Detail

Event Timeline

jasonliu created this revision.Jun 3 2020, 8:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2020, 8:32 AM
jasonliu edited reviewers, added: daltenty; removed: david-arm.Jun 3 2020, 8:53 AM
jasonliu set the repository for this revision to rG LLVM Github Monorepo.
jasonliu updated this revision to Diff 268223.Jun 3 2020, 8:55 AM

why the file llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-overflow.ll so large ,it can not be loaded?

jasonliu added a comment.EditedJun 3 2020, 2:23 PM

why the file llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-overflow.ll so large ,it can not be loaded?

I was able to load it by clicking Show File Contents.
I'm open to any ideas that could greatly reduce the file size and still trigger the error I put in.

daltenty added inline comments.Jun 4 2020, 7:39 AM
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-overflow.ll
6 ↗(On Diff #268223)

We're able to sed and friends in tests, so I think we can do an expansion based on @hubert.reinterpretcast original preprocessor example. Replacing the repeated line with 'A' and using something like this seems like it would do the trick:

sed "s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;s/A/ABA/g;" %s| tr B "\n" | sed "s/A/call\ void\ bitcast\ \(void (...)*\ @foo\ to\ void\ ()*)()/g"|llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr4 -mattr=-altivec -filetype=obj -o %t.o
jasonliu updated this revision to Diff 268542.Jun 4 2020, 11:24 AM

Use sed to generate the test.

jasonliu marked an inline comment as done.Jun 4 2020, 11:26 AM
jasonliu added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-overflow.ll
6 ↗(On Diff #268223)

Thanks for the suggestion.
I could not 'sed' the same file that the 'sed' command resides. So I created an input file instead.

llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-overflow.ll
6 ↗(On Diff #268223)

Would grep -v have helped there?

jasonliu marked an inline comment as done.Jun 4 2020, 2:49 PM
jasonliu added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-overflow.ll
6 ↗(On Diff #268223)

Hmm... I'm not sure how grep -v would help.
The problem I'm seeing is:
when this file get passed into the sed command, sed command would replace the A recursively inside this sed command, which would result in an output file like:

;; This test generates more than 65535 relocation entries in a single section,
;; which would trigger an overflow section to be generated in 32-bit mode.
;; Since overflow section is not supported yet, we will emit an error instead of
;; generating an invalid binary for now.

; RUN: sed "s/call void bitcast (void (...)* @foo to void ()*)()
call void bitcast (void (...)* @foo to void ()*)()
call void bitcast (void (...)* @foo to void ()*)()
call void bitcast (void (...)* @foo to void ()*)()
.... 
/g;" \
; RUN:     %p/Inputs/reloc-sed.ll | tr B "\n" | \
....
jasonliu updated this revision to Diff 268634.Jun 4 2020, 6:44 PM

Remove input files and use grep -v

llvm/lib/MC/XCOFFObjectWriter.cpp
743

Semantically, this is not UINT16_MAX but an XCOFF-defined constant. Otherwise, the maximum representable value would be UINT16_MAX itself and not one less. RelocOverflow is currently defined in lib/Object/XCOFFObjectFile.cpp, but it should probably be defined in include/llvm/BinaryFormat/XCOFF.h.

744

Move the check to before the addition; use: Section->RelocationCount >= UINT16_MAX - Csect.Relocations.size(). After said move, the two if statements can be merged.

llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-overflow.ll
7 ↗(On Diff #268634)

I suggest testing exactly 65535:

; RUN: grep -v RUN: %s | \
; RUN:   sed >%t.overflow.ll 's/SIZE/65535/;s/MACRO/#/;s/#/################/g;s/#/################/g;s/#/################/g;s/#/################/g;s/#/#_/g;s/_#_\([^#]\)/\1/;s/_/, /g;s/#/i8* @c/g;'
; RUN: not --crash llc -verify-machineinstrs -mtriple powerpc-ibm-aix-xcoff \
; RUN:     -mcpu=pwr4 -mattr=-altivec -filetype=obj -o %t.o %t.overflow.ll 2>&1 | \
; RUN:   FileCheck --check-prefix=XCOFF32 %s
; XCOFF32: LLVM ERROR: relocation entries overflowed; overflow section is not implemented yet

; RUN: not --crash llc -verify-machineinstrs -mtriple powerpc64-ibm-aix-xcoff \
; RUN:     -mcpu=pwr4 -mattr=-altivec -filetype=obj -o %t.o %t.overflow.ll 2>&1 | \
; RUN:   FileCheck --check-prefix=XCOFF64 %s
; XCOFF64: LLVM ERROR: 64-bit XCOFF object files are not supported yet.

@c = external global i8, align 1
@arr = global [SIZE x i8*] [MACRO], align 8

Also, I think we should test for 65534.

jasonliu updated this revision to Diff 268816.Jun 5 2020, 8:32 AM

Address Hubert's comment.

jasonliu marked an inline comment as done.Jun 5 2020, 8:35 AM
jasonliu added inline comments.
llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-overflow.ll
7 ↗(On Diff #268634)

Thanks. I was pleasantly surprised that the suggested test(4 llc invocation) finished within 30 seconds.

jasonliu updated this revision to Diff 268823.Jun 5 2020, 8:48 AM
jasonliu marked 2 inline comments as done.
daltenty accepted this revision.Jun 8 2020, 7:19 AM

LGTM

llvm/lib/MC/XCOFFObjectWriter.cpp
748

nit: Capitalization and period.

This revision is now accepted and ready to land.Jun 8 2020, 7:19 AM
jasonliu marked an inline comment as done.Jun 8 2020, 7:45 AM
jasonliu added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
748

https://llvm.org/docs/CodingStandards.html#error-and-warning-messages
ask us not to capitalize first letter and end with period.

LGTM with minor comments.

llvm/test/CodeGen/PowerPC/aix-xcoff-huge-relocs.ll
9

Minor nit: Not sure if we want to align -mcpu with the llc options?

22

Same comment.

25

Same comment.

32

I think we'd need the NOT lines after the other lines as well.

This revision was automatically updated to reflect the committed changes.
jasonliu marked 4 inline comments as done.Jun 8 2020, 2:12 PM