search

ARM-software / astc-encoder

The Arm ASTC Encoder, a compressor for the Adaptive Scalable Texture Compression data format.
https://developer.arm.com/graphics
Apache License 2.0
1.08k stars 241 forks source link

Emscripten SAFE_HEAP reports unaligned access in compute_quantized_weights_for_decimation. #462

Closed MarkCallow closed 6 months ago

MarkCallow commented 7 months ago

in astcenc_ideal_endpoints_and_weights.cpp.

I can't provide a source line number at present because I am having a problem with that level of debugging.

To reproduce compile the library and an encoder app that uses it with Emscripten specifying -s SAFE_HEAP=1 and no code optimization options. Run the app and observe the exception.

solidpixel commented 7 months ago

Never tried Emscripten, so "will look" but fair warning that it might take a while before I get around to it.

MarkCallow commented 7 months ago

The only reason I opened the issue is because the warning says that on some native platforms unaligned accesses are very slow.

solidpixel commented 7 months ago

I generally try to avoid unaligned access, as we've seen people try to run this on Arm systems with unaligned access disabled so it will fault if you try.

solidpixel commented 7 months ago

Looks like UBSAN is flagging a few cases where we pass unaligned addresses into SIMD intrinsics. The intrinsics in question explicitly allow unaligned addresses, so this isn't necessarily a problem, but would be good to avoid if we can.

env UBSAN_OPTIONS=print_stacktrace=true ./bin/astcenc-avx2 -tl in.png out.png 6x6 -medium

solidpixel commented 7 months ago

@MarkCallow I'm in two minds about this one, given that this only impacts SIMD intrinsics which explicitly allow unaligned access, but can you try this branch:

https://github.com/ARM-software/astc-encoder/tree/unaligned_ubsan

... which at least seems to make UBSAN happy for the SIMD builds.

There is no measurable impact on performance (at least on a single x86-64 machine I tested on), as this is not impacting loads we expect to hit a lot in core loops.

solidpixel commented 7 months ago

Decided that a clean UBSAN was worth the cost, as it's a useful tool, so I've merged the branch into main. Can you let me know if this solves the problem - I think it should.

MarkCallow commented 7 months ago

Can you let me know if this solves the problem - I think it should.

Sorry for the delay. I'll test it as soon as I have time.

solidpixel commented 7 months ago

No need to apologize - there is no hurry =)

MarkCallow commented 6 months ago

Can you let me know if this solves the problem - I think it should.

Yes. It solved the problem. I no longer get the exception when SAFE_HEAP is defined.

When do you expect to release 4.8.0?

solidpixel commented 6 months ago

When do you expect to release 4.8.0?

I didn't really have a date in mind. If you need one to pull for KTX-Tools, I can do it this week.

MarkCallow commented 6 months ago

I didn't really have a date in mind. If you need one to pull for KTX-Tools, I can do it this week.

I have quite a bit more work before our next release so there is no huge rush but if you can do it this week I can pull it now while there is no danger of it being a critical path item.

solidpixel commented 6 months ago

OK, I'll see what I can get done. Should be this week, but we're rolling out a new CI for this based on public GitHub Actions, so might be some teething issues =)

MarkCallow commented 6 months ago

OK, I'll see what I can get done. Should be this week.

Thanks Pete. I've subscribed to notifications of new releases.