Page MenuHomePhabricator

[DebugInfo] Fix LineTest byteswap for cross-targeting builds
ClosedPublic

Authored by daltenty on Mon, Jun 29, 5:30 PM.

Details

Summary

The byte swap fix for big endian hosts in 9782c922cb21 (for D81570)
swaps based on the host endianess, but for cross-targeting builds (i.e.
big endian host targeting little endian) the host-endianess won't
necessarily match the generated DWARF. This change updates the test
to use symmetrical constants so the results aren't endian dependent.

Diff Detail

Event Timeline

daltenty created this revision.Mon, Jun 29, 5:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jun 29, 5:30 PM

The patch looks appropriate, but now I think that maybe it would have been enough to simply use symmetrical constants for these particular tests?

The patch looks appropriate, but now I think that maybe it would have been enough to simply use symmetrical constants for these particular tests?

Testing endianness behaviour of the code under test is not what the original change was about, so getting rid of this byte-swapping code entirely would make the testing simpler, I agree. @daltenty, are you happy to do that? It should be a fairly straightforward change, I believe.

daltenty added a comment.EditedTue, Jun 30, 7:20 AM

The patch looks appropriate, but now I think that maybe it would have been enough to simply use symmetrical constants for these particular tests?

Testing endianness behaviour of the code under test is not what the original change was about, so getting rid of this byte-swapping code entirely would make the testing simpler, I agree. @daltenty, are you happy to do that? It should be a fairly straightforward change, I believe.

That makes sense to me, I will update to remove the byteswap and add a set of symmetrical constants for the big-endian case.

daltenty updated this revision to Diff 274615.Tue, Jun 30, 2:17 PM
  • Use an Optional big-endian output string rather than trying to byte swap

The patch looks appropriate, but now I think that maybe it would have been enough to simply use symmetrical constants for these particular tests?

Testing endianness behaviour of the code under test is not what the original change was about, so getting rid of this byte-swapping code entirely would make the testing simpler, I agree. @daltenty, are you happy to do that? It should be a fairly straightforward change, I believe.

That makes sense to me, I will update to remove the byteswap and add a set of symmetrical constants for the big-endian case.

Ended up adding an optional big endian version of the message instead, as I think it comes out a bit cleaner than constructing an optional version of the input using the symmetrical constant, but let me know if you have a preference for that version instead.

jhenderson added inline comments.Wed, Jul 1, 12:41 AM
llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
1475

It's possible you may have misunderstood what we were suggesting. Rather than adding another argument, I think @ikudrin and I were thinking of changing the 0x1234567890abcdef to 0x1234567878563412, and so on in the other tests, so that the error message would be the same regardless of endianness. That way the test is completely agnostic of endianness and there's no need to query the host/target.

1479–1480

This would be the same as above.

1524–1525

This would be 0x12343412.

ikudrin added inline comments.Wed, Jul 1, 2:01 AM
llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
1475

Right, that was the idea.

daltenty updated this revision to Diff 275131.Thu, Jul 2, 8:25 AM
  • Use symmetrical constants instead
daltenty marked 2 inline comments as done.Thu, Jul 2, 8:27 AM
daltenty added inline comments.
llvm/unittests/DebugInfo/DWARF/DWARFDebugLineTest.cpp
1475

Yes, I definitely did. This is a much better solution, updated as suggested.

daltenty edited the summary of this revision. (Show Details)Thu, Jul 2, 8:29 AM
ikudrin accepted this revision.Thu, Jul 2, 9:44 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Thu, Jul 2, 9:44 AM
This revision was automatically updated to reflect the committed changes.

Thanks for fixing this!