[PATCH] D98540: [M68k] Implement Disassembler

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: llvm/lib/Target/M68k/Disassembler/M68kDisassembler.cpp:448
+
+  Scratch = (Scratch << NumToRead) | Reader.readBits(NumToRead);
+}
----------------
ricky26 wrote:
> RKSimon wrote:
> > @ricky26 coverity is warning that the NumToRead = 32 case results in UB when shifting Scratch.
> > 
> > Could this be:
> > ```
> > Scratch = NumRead >= 32 ? 0 : (Scratch << NumToRead);
> > Scratch |= Reader.readBits(NumToRead);
> > ```
> Thanks, I always forget that shifting the full width is UB. (Is coverity something I should be watching for my changes?)
> 
> Does LLVM support host compilers with non-32-bit `int`s? If so, the logic will need to be more complicated.
> 
> Something like `Scratch = (NumRead < (sizeof(unsigned) * 8)) ? (Scratch << NumToRead) : 0;`?
> 
> If this isn't high priority I'll try and land a patch for it sometime over the coming week.
> 
> Thanks!
I don't think LLVM would survive building on non-32-bit int target......

Its not high priority, but I do try to keep an eye on static analysis reports as they do sometimes raise interesting issues. Potential out of range shift amount warnings are quite common in the llvm codebase.

You can request access to the coverity reports via: https://scan.coverity.com/projects/llvm


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98540/new/

https://reviews.llvm.org/D98540