Fix endianness handling in AVR MC

[serge-sans-paille]

Summary

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

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

[tstellar]

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

[efriedma]

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]

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

[serge-sans-paille]

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

[efriedma]

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.)

[serge-sans-paille]

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!

[tstellar]

Ping.

[dylanmckay]

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

[serge-sans-paille]

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

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

 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCFixup.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
+#include "llvm/Support/EndianStream.h"
 
 #define DEBUG_TYPE "mccodeemitter"
 
 #define GET_INSTRMAP_INFO
 #include "AVRGenInstrInfo.inc"
 #undef GET_INSTRMAP_INFO
 
 namespace llvm {
unsigned AVRMCCodeEmitter::getMachineOpValue(const MCInst &MI,
   assert(MO.isExpr());
 
   return getExprOpValue(MO.getExpr(), Fixups, STI);
 }
 
 void AVRMCCodeEmitter::emitInstruction(uint64_t Val, unsigned Size,
                                        const MCSubtargetInfo &STI,
                                        raw_ostream &OS) const {
-  const uint16_t *Words = reinterpret_cast<uint16_t const *>(&Val);
   size_t WordCount = Size / 2;
 
   for (int64_t i = WordCount - 1; i >= 0; --i) {
-    uint16_t Word = Words[i];
-
-    OS << (uint8_t) ((Word & 0x00ff) >> 0);
-    OS << (uint8_t) ((Word & 0xff00) >> 8);
+    uint16_t Word = (Val >> i * 16) & 0xFFFF;
+    support::endian::write(OS, Word, support::endianness::little);
   }
 }
 
 void AVRMCCodeEmitter::encodeInstruction(const MCInst &MI, raw_ostream &OS,
                                          SmallVectorImpl<MCFixup> &Fixups,
                                          const MCSubtargetInfo &STI) const {
   const MCInstrDesc &Desc = MCII.get(MI.getOpcode());