openexr: ensure struct is packed on PPC systems

[MarcusCalhoun-Lopez]

Description

Type(s)

• bugfix
• enhancement
• security fix

Tested on

macOS x.y
Xcode x.y / Command Line Tools x.y.z

Verification

Have you

• followed our Commit Message Guidelines?
• squashed and minimized your commits?
• checked that there aren't other open pull requests for the same change?
• referenced existing tickets on Trac with full URL?
• checked your Portfile with port lint --nitpick?
• tried existing tests with sudo port test?
• tried a full install with sudo port -vst install?
• tested basic functionality of all binary files?
• checked that the Portfile's most important variants haven't been broken?

[macportsbot]

Notifying maintainers:
@mascguy for port openexr.

[MarcusCalhoun-Lopez]

I do not see why this fix is necessary, but more tests succeeded with it.
Perhaps it is a bug in GCC 7?

[mascguy]

I do not see why this fix is necessary, but more tests succeeded with it. Perhaps it is a bug in GCC 7?

Sergey's previous work related to PPC is here, including links to discussion with upstream:

PR 18711 - openexr: fix defines in libdispatch-related patch

But if you've already reviewed everything, then no worries.

CC: @barracuda156

[MarcusCalhoun-Lopez]

I do not recall any mention of this issue, but I may have missed it.

[mascguy]

I do not recall any mention of this issue, but I may have missed it.

You'll want to review all of the discussion in Sergey's upstream issues:

https://github.com/AcademySoftwareFoundation/openexr/issues/created_by/barracuda156

[mascguy]

p.s. OpenEXR upstream has been very responsive, and committed at least one fix relative to Sergey's discussions. Awesome collaboration, to be sure!

[kencu]

@evanmiller and I went through a longish investigation of struct packing on ppc about two years ago building mozjs … probably relevant if we can find it

[barracuda156]

I do not see why this fix is necessary, but more tests succeeded with it.

Perhaps it is a bug in GCC 7?

I can run tests tomorrow with gcc12. (Now almost sleeping.)

[MarcusCalhoun-Lopez]

You'll want to review all of the discussion in Sergey's upstream issues:

I already have, but if this particular issue was mentioned, I did not recognize it.

[barracuda156]

You'll want to review all of the discussion in Sergey's upstream issues:

I already have, but if this particular issue was mentioned, I did not recognize it.

Me neither, but don’t count on my brain right now )

[mascguy]

You'll want to review all of the discussion in Sergey's upstream issues:

I already have, but if this particular issue was mentioned, I did not recognize it.

I'm quite certain that packing-related discussion occurred in one of those, probably those related to test failures.

[kencu]

I found the powerpc struct packing analysis evan and I did before here:

https://trac.macports.org/ticket/63490

[MarcusCalhoun-Lopez]

https://trac.macports.org/ticket/63490

Thank you for the link and the analysis.
It seems like libdazzle uses the same solution/workaround as this PR.

[barracuda156]

@kencu Thank you for the link, I need to go through that!

@MarcusCalhoun-Lopez Could you post test results from gcc7 (on Leopard, I assume?)?

[MarcusCalhoun-Lopez]

I have already reduced the problem to the smallest test case.
Please see the comments in the patch file.

To summarize: Using GCC7 on PPC Mac OS X Leopard (10.5),
#pragma pack(push, 1) does not always force packing.
__attribute__((packed, aligned(1))) does force packing.
OpenEXR requires packing for certain data structures.

[barracuda156]

@MarcusCalhoun-Lopez Ok let me just try your examples. I am curious to see if that was fixed in GCC.

[MarcusCalhoun-Lopez]

@MarcusCalhoun-Lopez Ok let me just try your examples. I am curious to see if that was fixed in GCC.

The system compiler (g++-4.2) and -arch ppc64 with any compiler do not have this problem, which is a bit odd.

[barracuda156]

@MarcusCalhoun-Lopez

sergey-fedorovs-power-mac-g5:~ svacchanda$ /opt/local/bin/g++-mp-11 main.cxx && ./a.out
12 9 9
sergey-fedorovs-power-mac-g5:~ svacchanda$ /opt/local/bin/g++-mp-12 main.cxx && ./a.out
12 9 9
sergey-fedorovs-power-mac-g5:~ svacchanda$ g++ main.cxx && ./a.out
9 9 9

Has this been filed to GCC upstream?

[MarcusCalhoun-Lopez]

Has this been filed to GCC upstream?

No, I am afraid not.

[barracuda156]

No, I am afraid not.

Should I open a ticket on Bugzilla or you wanna do it? (tag Iain if you do).

P. S. And I guess we need to pass the fix to openexr upstream too, to begin with. GCC will take forever, even if they care to fix it.

[MarcusCalhoun-Lopez]

No, I am afraid not.

Should I open a ticket on Bugzilla or you wanna do it? (tag Iain if you do).

If you wouldn't mind, I would greatly appreciate it.
It would take me a while to learn how to submit bug reports to GCC.

P. S. And I guess we need to pass the fix to openexr upstream too, to begin with. GCC will take forever, even if they care to fix it.

I was a little hesitant to talk to OpenEXR upstream about this since, as far as I can tell, it a GCC issue on PowerPC.
However, if they have insights, that would be great.

[barracuda156]

I was a little hesitant to talk to OpenEXR upstream about this since, as far as I can tell, it a GCC issue on PowerPC.
However, if they have insights, that would be great.

If it does not break anything else, I reckon there is no reason not to add it. If it does, then adding it conditionally is isn’t troublesome either, I hope.

If you wouldn't mind, I would greatly appreciate it.

Sure, will open an issue now.

UPD. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110044

[kencu]

gcc will just say "If you want a struct packed, then declare it as packed".

:>

typedef struct __attribute__((packed))

[kencu]

apple added various extensions to gcc to suit their purposes -- such things are in gcc-4.2 (from Apple) but not necessarily in the main gcc codebase. So Apple's gcc (and Apple's clang and llvm, for that matter) are a bit different from mainline gcc. These extensions may well not be appropriate to put in the main gcc codebase. These are not necessarily gcc bugs, or anyone's bugs -- just have to learn the proper way to do things.

[barracuda156]

@kencu Let’s see what upstream says on this one.

[MarcusCalhoun-Lopez]

Although we still have to work with upstream (OpenEXR and/or GCC), I do not believe there are any objections to the PR itself, so I will go ahead and merge.

With this PR, on PPC systems, 7 tests still fail for a 94% success rate.
It might be a while before I can look into the remaining seven tests.

[barracuda156]

@MarcusCalhoun-Lopez GCC upstream is working on the issue, Iain may have a solution. To be updated.

Could you submit a fix to openexr upstream in the meanwhile?

[MarcusCalhoun-Lopez]

@MarcusCalhoun-Lopez GCC upstream is working on the issue, Iain may have a solution. To be updated.

Could you submit a fix to openexr upstream in the meanwhile?

I would be happy to submit a PR to OpenEXR, but may I humbly suggest we wait to hear back from GCC first?
After all, the OpenEXR code does have #pragma pack(push, 1) already, and if we can better understand why the pragma is not being respected, then we might better know the best solution.

[barracuda156]

@MarcusCalhoun-Lopez Yeah, no hurry here, sure.