Fix packed LUID conversions #2559
Conversation
|
Author mattst88 not on autobuild list. Waiting for curator authorization before starting CI build. |
mattst88
force-pushed
the
fix-packed-luid-conversions
branch
from
96c61af to
fdef2c9
Compare
|
Author mattst88 not on autobuild list. Waiting for curator authorization before starting CI build. |
mattst88
force-pushed
the
fix-packed-luid-conversions
branch
from
fdef2c9 to
cfc1b25
Compare
|
Author mattst88 not on autobuild list. Waiting for curator authorization before starting CI build. |
mattst88
force-pushed
the
fix-packed-luid-conversions
branch
from
cfc1b25 to
7fcf44a
Compare
|
Author mattst88 not on autobuild list. Waiting for curator authorization before starting CI build. |
|
The code looks clean to me. I'm starting a CI build in order to see the impacts on Windows, which the author is unable to test. |
|
CI gfxreconstruct build queued with queue ID 601414. |
|
CI gfxreconstruct build # 8350 running. |
|
Fails to build on Wndows. A typical |
|
CI gfxreconstruct build # 8350 failed. |
@lunarpapillo The GitHub CI build also builds on Windows and I've enabled several times and the author has been able to provide several rounds of attempts to pass CI through branch pushes. There's no need to tie up internal Jenkins CI results until the GH CI runs succeed. |
mattst88
force-pushed
the
fix-packed-luid-conversions
branch
from
7fcf44a to
68b2cad
Compare
|
Author mattst88 not on autobuild list. Waiting for curator authorization before starting CI build. |
|
Could use another CI workflow approval. Hopefully this is the last time ;) |
|
CI gfxreconstruct build queued with queue ID 603508. |
|
CI gfxreconstruct build # 8369 running. |
|
CI gfxreconstruct build # 8369 failed. |
|
Hi, @mattst88 ! I really appreciate this PR and I want to merge it but Our GitHub CI resources are not infinite. I don't think I want to authorize more runs until you can resolve the Windows compilation issue. Can you find or borrow a Windows machine to resolve the Windows build? Thank you! |
mattst88
force-pushed
the
fix-packed-luid-conversions
branch
from
68b2cad to
8b1b910
Compare
|
Author mattst88 not on autobuild list. Waiting for curator authorization before starting CI build. |
Sorry. I'm not interested or invested enough to spend the time getting a Windows development environment going so I can fix this. It's just something I noticed and figured wouldn't be too difficult to fix, but if it's too expensive to run the CI a few times and no one who actually works on the project can spare the time to figure out what |
|
I will drop the patch that touches Windows-only code and leaves it obviously broken, since it's not worth the CI costs to fix. |
This solves the compile error when building with
`-Werror=strict-aliasing`.
```
/home/mattst88/projects/gfxreconstruct/framework/encode/parameter_encoder.h: In member function ‘void gfxrecon::encode::ParameterEncoder::EncodeLUIDValue(LUID)’:
/home/mattst88/projects/gfxreconstruct/framework/encode/parameter_encoder.h:80:53: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
80 | void EncodeLUIDValue(LUID value) { EncodeValue(*reinterpret_cast<int64_t*>(&value)); }
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
Closes: LunarG#2358
mattst88
force-pushed
the
fix-packed-luid-conversions
branch
from
8b1b910 to
9e55017
Compare
|
Author mattst88 not on autobuild list. Waiting for curator authorization before starting CI build. |
|
This is ready for a final CI run. |
|
@bradgrantham-lunarg: ping? |
|
@bradgrantham-lunarg: ping²? |
4 participants
Two commits. I'm on Linux, so I'm unable to test the Windows-only changes. Hopefully CI will handle that.
Add and use packed_luid() function
Nearly all of the open-coded implementations were wrong in multiple ways:
Use packed_luid() to avoid aliasing violation
This solves the compile error when building with
-Werror=strict-aliasing.Closes: #2358
cc: @igorbrsn, @JerryAMD, @rurra-amd because they all introduced instances of
<< 31cc: @jzulauf-lunarg because he introduced the aliasing violation and then dutifully ignored issue #2358