This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by DiggerLin on Dec 23 2019, 10:48 AM.

Details

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.

Diff Detail

Event Timeline

DiggerLin created this revision.Dec 23 2019, 10:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2019, 10:48 AM
daltenty added inline comments.Dec 30 2019, 12:26 PM
llvm/test/CodeGen/PowerPC/aix-return55.ll
19

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).

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

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.

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

DiggerLin planned changes to this revision.Jan 13 2020, 12:39 PM
DiggerLin updated this revision to Diff 245666.Feb 20 2020, 8:44 AM
DiggerLin marked 2 inline comments as done.
daltenty accepted this revision.Feb 24 2020, 7:09 AM

Minor nit, but otherwise LGTM

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

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

20

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

This revision is now accepted and ready to land.Feb 24 2020, 7:09 AM
hubert.reinterpretcast requested changes to this revision.Feb 24 2020, 4:22 PM
hubert.reinterpretcast added inline comments.
llvm/test/CodeGen/PowerPC/aix-return55.ll
38

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.

This revision now requires changes to proceed.Feb 24 2020, 4:22 PM
DiggerLin updated this revision to Diff 246595.Feb 25 2020, 3:51 PM
DiggerLin retitled this revision from [AIX][XCOFF] add test for raw text section content and test section header to [AIX][XCOFF] Fixed a bug of XCOFFObjectWriter.cpp and add test for raw text section content and test section header.
DiggerLin edited the summary of this revision. (Show Details)
DiggerLin marked an inline comment as done.

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

DiggerLin marked 2 inline comments as done.Feb 25 2020, 3:55 PM
DiggerLin added inline comments.
llvm/test/CodeGen/PowerPC/aix-return55.ll
20

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

DiggerLin updated this revision to Diff 246609.Feb 25 2020, 5:46 PM
DiggerLin marked an inline comment as done.
DiggerLin retitled this revision from [AIX][XCOFF] Fixed a bug of XCOFFObjectWriter.cpp and add test for raw text section content and test section header to [AIX][XCOFF] add test for raw text section content and test section header and Fixed a bug of XCOFFObjectWriter.cpp .Feb 26 2020, 6:09 AM
llvm/test/CodeGen/PowerPC/aix-return55.ll
38

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.

DiggerLin retitled this revision from [AIX][XCOFF] add test for raw text section content and test section header and Fixed a bug of XCOFFObjectWriter.cpp to [AIX][XCOFF][BUG-FIXED]Fixed a bug of XCOFFObjectWriter.cpp and add test for raw text section content and test section header.Feb 27 2020, 11:46 AM

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 retitled this revision from [AIX][XCOFF][BUG-FIXED]Fixed a bug of XCOFFObjectWriter.cpp and add test for raw text section content and test section header to [AIX][XCOFF][AIX] Fix XCOFFObjectWriter assertion failure with alignment-related gap and improve text section output testing.Feb 27 2020, 1:32 PM

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 added inline comments.Feb 28 2020, 8:20 AM
llvm/lib/MC/XCOFFObjectWriter.cpp
440 ↗(On Diff #246609)

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

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

21

This line and the next line is not aligned.

DiggerLin marked an inline comment as done.

address comment

llvm/lib/MC/XCOFFObjectWriter.cpp
442 ↗(On Diff #247306)

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

444 ↗(On Diff #247306)

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 marked 5 inline comments as done.Feb 28 2020, 12:52 PM
DiggerLin added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
442 ↗(On Diff #247306)

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

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

these are caused by llvm-objdump -D

jasonliu accepted this revision.Feb 28 2020, 1:02 PM

LGTM except the minor nit.

llvm/lib/MC/XCOFFObjectWriter.cpp
442 ↗(On Diff #247306)

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

You are replying to the wrong thread.

llvm/lib/MC/XCOFFObjectWriter.cpp
442 ↗(On Diff #247306)

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

DiggerLin updated this revision to Diff 247636.Mar 2 2020, 7:10 AM
DiggerLin marked 8 inline comments as done.

address comment

hubert.reinterpretcast retitled this revision from [AIX][XCOFF][AIX] Fix XCOFFObjectWriter assertion failure with alignment-related gap and improve text section output testing to [AIX][XCOFF] Fix XCOFFObjectWriter assertion failure with alignment-related gap and improve text section output testing.Mar 2 2020, 7:52 AM

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

This revision is now accepted and ready to land.Mar 2 2020, 9:53 AM
This revision was automatically updated to reflect the committed changes.