pnggroup / libpng

LIBPNG: Portable Network Graphics support, official libpng repository
http://libpng.sf.net
Other
1.28k stars 620 forks source link

BUILD BREAK: arm/palette_neon_intrinsics.c fails to compile with -Wcast-align #605

Closed jbowler closed 3 weeks ago

jbowler commented 1 month ago

This is a clear problem because the code is apparently taking advantage of spurious memory alignment; this may change on some systems and may potentially crash on some systems.

The code in question added a png_bytep called riffled_palette to pngstruct.h which is, in fact, not used as a png_bytep rather it is a png_uint_32p.

I think the fix is just to make the declaration of riffled_palette png_uint_32p but doing this requires significant changes to the supplied code (i.e. a quick hack may or may not be correct) so requires the maintainer.

It also seems to be the case that the code does not require any more than byte alignment, so maybe the fix is to just declare the riffled_palette a png_voidp.

@richard-townsend-arm as the maintainer, please comment. @ctruta please also check #266 for background, and your comments at the end. Perhaps this is something that can be undealt with in 1.8?

ctruta commented 4 weeks ago

With the caveat that I'm far from being a SIMD expert, and under the assumption that -Wcast-align is a legitimate warning and not a false positive, I concur with the idea of changing the type of riffled_palette from png_bytep to png_uint_32p.

@ctruta please also check https://github.com/pnggroup/libpng/issues/266 for background, and your comments at the end. Perhaps this is something that can be undealt with in 1.8?

So (I said) I pushed a solution that "works for me", and that may still be true. But then, I doubt that changing the pointer type from png_bytep to png_voidp will actually solve the underlying problem -- it will merely make the warning go away.

But then, again, I'm absolutely not a SIMD expert. @richard-townsend-arm help please 😄

jbowler commented 4 weeks ago

The alignment warning is both legitimate and a false positive; the pointer was allocated by malloc(1024) but the compiler cannot know that. The problem is that -Wcast-align is one of the warnings that have been generally supported (see scripts/makefile.std) and if I enable it this, and only this, piece of code breaks the build on ARM: MIPS is a different situation.

It can be fixed by "png_aligncast", which is the way it should have been written in the first place.

I suggested (void*) both because it makes the code non-ARM-specific and because it seemed that none of the pointers have an alignment requirement. I know enough about SIMD to see that the pointers aren't aligned in some cases and I tested the case (row pointer misalignment) that was easy to test and it seemed to work.

It's a maintenance issue. png_aligncast is fine by me if a maintainer doesn't show up. After all it will only crash on ARM Ltd designed systems and they wrote the code. It's also easy to disable (not quite as easy as it should be, but still easy.)

jbowler commented 3 weeks ago

I created #607 to fix this. The patch could also be applied to libpng16 - it is harmless because it does what was already being done it just does it in a way that quells the warning. Safe because the pointer comes from malloc. Other, maybe better, fixes require more lines changed and more review.

ctruta commented 3 weeks ago

Applied, thank you.