nativeGen/X86: Ensure that info tables are aligned
AbandonedPublic

Authored by bgamari on Jan 25 2016, 4:20 PM.

Details

Reviewers
olsner
austin
rwbarton
Trac Issues
#11486
Summary

This was broken in 4a32bf925b8aba7885d9c745769fe84a10979a53, meaning
that info tables and subsequent code are no longer guaranteed to have the recommended
alignment. This restores the alignment to four-bytes.
Fixes Trac #11486

Test Plan

Validate and nofib comparison

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
master
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 8380
Build 10425: arc lint + arc unit
bgamari updated this revision to Diff 6432.Jan 25 2016, 4:20 PM
bgamari retitled this revision from to nativeGen/X86: Ensure that info tables are aligned.
bgamari updated this object.
bgamari edited the test plan for this revision. (Show Details)
bgamari added reviewers: olsner, rwbarton.
bgamari updated the Trac tickets for this revision.
bgamari updated this object.Jan 25 2016, 4:22 PM
bgamari edited edge metadata.
rwbarton requested changes to this revision.Jan 25 2016, 4:45 PM
rwbarton edited edge metadata.

I took a look over the -split-sections commit. In the analogous spot in the PPC and SPARC backends, a pprSectionHeader Text $$ turned into pprSectionAlign (Section Text info_lbl) $$. Now, I have no idea if those backends were really tested... but a duplicate .text symbol directive shouldn't cause any harm (it was in the old assembly output anyways). And the info table should be aligned to 8 bytes on x86_64. Well, I don't know if it's really best, but again it was the old bevahior.

So, I would rather just add pprSectionAlign (Section Text info_lbl) $$ here, instead.

I hope @olsner can comment on whether this was just an oversight, or intentional for some reason.

This revision now requires changes to proceed.Jan 25 2016, 4:45 PM
olsner edited edge metadata.Jan 25 2016, 5:15 PM

Yeah, this was just an oversight... I think I missed that this was here to align the info tables, and thought it was just printing a redundant section header.
The corresponding code in SPARC/PowerPC also ended up a bit broken unfortunately - with -split-sections enabled, printing a new section header here is not quite a no-op, as it switches to a new .text subsection. That makes pprSizeDecl generate an assembler error, because the end-label and start-label are not in the same section anymore.

I think we should split up the alignment printing from pprSectionAlign so we can get the same "text section" alignment for info tables without switching section.

rwbarton added a comment.EditedJan 25 2016, 5:27 PM
In D1846#54368, @olsner wrote:

Yeah, this was just an oversight... I think I missed that this was here to align the info tables, and thought it was just printing a redundant section header.
The corresponding code in SPARC/PowerPC also ended up a bit broken unfortunately - with -split-sections enabled, printing a new section header here is not quite a no-op, as it switches to a new .text subsection. That makes pprSizeDecl generate an assembler error, because the end-label and start-label are not in the same section anymore.

Oh, I didn't realize the size of the top-level info symbol extended over all the basic blocks. Fine, there is no point in splitting the basic blocks into different sections anyways.

I think we should split up the alignment printing from pprSectionAlign so we can get the same "text section" alignment for info tables without switching section.

Yes, this looks sensible and easy.

Alright, sounds like a plan. Would someone like to pick this up?

bgamari abandoned this revision.Jan 26 2016, 2:24 AM