This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by DiggerLin on Nov 29 2019, 7:46 AM.

Details

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

Diff Detail

Event Timeline

DiggerLin created this revision.Nov 29 2019, 7:46 AM
DiggerLin edited reviewers, added: xingxue; removed: jdoerfert.Nov 29 2019, 7:47 AM
daltenty requested changes to this revision.Dec 3 2019, 8:32 AM

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.

This revision now requires changes to proceed.Dec 3 2019, 8:32 AM
DiggerLin marked 5 inline comments as done.Dec 5 2019, 8:41 AM
DiggerLin added inline comments.
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 updated this revision to Diff 232359.Dec 5 2019, 8:42 AM
DiggerLin marked 2 inline comments as done.

change test case based on comment.

DiggerLin marked an inline comment as done.Dec 5 2019, 8:43 AM
jasonliu added a comment.EditedDec 5 2019, 9:06 AM

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.

DiggerLin updated this revision to Diff 232568.Dec 6 2019, 7:56 AM
DiggerLin retitled this revision from [XCOFF] fixed a bug of XCOFFObjectFile.cpp and adding new test case to verify one mergeable string for xcoffobjectfile to [BUG-FIX][XCOFF] fixed a bug of XCOFFObjectFile.cpp when there is padding at the last csect of a sections.
DiggerLin edited the summary of this revision. (Show Details)
DiggerLin removed a reviewer: jdoerfert.
DiggerLin edited the summary of this revision. (Show Details)
daltenty added inline comments.Dec 6 2019, 1:58 PM
llvm/lib/MC/XCOFFObjectWriter.cpp
359

This can be merged into the loop body now

366

Combine this with the declaration

DiggerLin updated this revision to Diff 232916.Dec 9 2019, 1:00 PM
DiggerLin marked 2 inline comments as done.
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 marked 3 inline comments as done.Dec 9 2019, 1:55 PM
DiggerLin added inline comments.
llvm/lib/MC/XCOFFObjectWriter.cpp
366

thank for reminding me the problem.

DiggerLin updated this revision to Diff 232941.Dec 9 2019, 1:56 PM
DiggerLin marked an inline comment as done.

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 accepted this revision.Dec 10 2019, 8:07 AM

Other than minor nit, LGTM

This revision is now accepted and ready to land.Dec 10 2019, 8:07 AM
This revision was automatically updated to reflect the committed changes.