This is an archive of the discontinued LLVM Phabricator instance.

Fix endianness handling in AVR MC
ClosedPublic

Authored by serge-sans-paille on Sep 23 2019, 11:11 AM.

Details

Summary

Replace reinterpret_cast + hand-written byteswap by proper type conversion + generic byteswap

Fix https://bugs.llvm.org/show_bug.cgi?id=43384

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2019, 11:11 AM

@dylanmckay Why is the byte-swapping need here in the first place?

This is still wrong. The problem isn't the explicit swap that's being changed; it's the reinterpret_cast which implicitly depends on endianness (and breaks strict aliasing).

Probably clearer to explicitly write out the conversion from uint64_t explicitly. For example, instead of uint16_t Word = Words[i];, instead write uint16_t Word = (uint16_t)(Val >> (WordCount * 16));.

serge-sans-paille edited the summary of this revision. (Show Details)

Remove reinterpret_cast as suggested by @efriedma
Force conversion native -> little

This patche validates on alla rchitectures supported by fedora, see https://koji.fedoraproject.org/koji/taskinfo?taskID=37835735

How have you been testing this? How many regression test failures are there on s390x? The first version of the patch should not have passed all the regression tests on a big-endian target, I think. (In particular, it should not have worked for 32-bit instructions, like llvm/test/MC/AVR/inst-jmp.s).

Probably want to wait for Dylan to comment, but looks fine. (Changing the code to use support::endian::write isn't necessary, but it's more readable.)

How have you been testing this?

Rebuilding llvm package with AVR support on alla supported arch using Fedora buildsystem

How many regression test failures are there on s390x? The first version of the patch should not have passed all the regression tests on a big-endian target, I think. (In particular, it should not have worked for 32-bit instructions, like llvm/test/MC/AVR/inst-jmp.s).

Yeah, the first version of the patch was not tested correctly (I was explicilty removing AVR from the targets for s390x). When tested correctly, most MC/AVR tests where failing. This is no longer the case (tests activated and pass).

Probably want to wait for Dylan to comment, but looks fine. (Changing the code to use support::endian::write isn't necessary, but it's more readable.)

OK, let's wait for @dylanmckay input for a few days!

dylanmckay accepted this revision.Nov 24 2019, 9:51 PM

Nice patch, sorry for taking so long to getting around to this.

@dylanmckay Why is the byte-swapping need here in the first place?

It was quite a long time ago when written, I can't recall this particular logic. I suspect it was a misunderstanding on my part of the format of uint64_t Val.

I've left a couple comments, I'm happy to accept this patch now with the two minor nitpicks fixed.

I've tested this locally and it doesn't cause any regressions, I agree that the reinterpret cast in its prior form makes incorrect assumptions about the host byte ordering, good catch @serge-sans-paille!

llvm/lib/Target/AVR/MCTargetDesc/AVRMCCodeEmitter.cpp
29

Place #includes in alphabetical order CodingStandards

275

Nitpick: I recommend adding parentheses around i * 16 so the reader doesn't have to be conscious about precedence rules to mentally parse this

This revision is now accepted and ready to land.Nov 24 2019, 9:51 PM

Thanks for the review! I'll do the update and merge the patch.

This revision was automatically updated to reflect the committed changes.