[PATCH] D84913: [libFuzzer] Enable for SystemZ

Kostya Serebryany via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 30 09:43:15 PDT 2020


kcc added inline comments.


================
Comment at: compiler-rt/lib/fuzzer/FuzzerTracePC.h:198
+    if (LargeType Bundle = *reinterpret_cast<const LargeType *>(P)) {
+#if __BYTE_ORDER == __BIG_ENDIAN
+      Bundle = Bswap(Bundle);
----------------
iii wrote:
> kcc wrote:
> > iii wrote:
> > > kcc wrote:
> > > > please avoid #ifdefs inside functions. 
> > > > If there is some logic that needs to depend on the platform, it needs to reside in a separate dedicated function. 
> > > > 
> > > > FuzzerBuiltins.h is probably the place for such a function. 
> > > > 
> > > > Also, what about 
> > > >        #include <endian.h>
> > > >        uint64_t htole64(uint64_t host_64bits);
> > > > is that portable? 
> > > > 
> > > I think this might not work as expected on 32-bit systems. I'll add a separate function.
> > No strong preference. 
> > I've just checked, on Linux x86_64: it inlines and works great. 
> > Dunno if it will have portability issues. 
> > 
> > ```
> > % cat x.cc 
> >  #include <endian.h>
> > #include <stdint.h>
> > 
> > uint64_t foo(uint64_t x) {
> >   return htole64(x);
> > }
> > % clang -O2 -S -o - x.cc -m32
> >         movl    4(%esp), %eax
> >         movl    8(%esp), %edx
> >         retl
> > 
> > % clang -O2 -S -o - x.cc 
> > 
> >         movq    %rdi, %rax
> >         retq
> > ```
> > 
> > `htobe64` also looks nice, it translates into one `bswapq` on 64-bit and two `bswapl` on 32-bit, 
> > i.e. in your case this is what `htole64` will do. 
> My thinking is that LargeType is uintptr_t, so it will be uint32_t on a 32-bit system.
> So if we have something like
> 
> uint32_t Bundle = 0xAABBCCDD;
> 
> htole64(Bundle) would give us (uint64_t)0xDDCCBBAA00000000, and then when we cast it back to uint32_t, we'll get just 0. Isn't that right?
> 
> So the idea I'm currently regtesting is:
> 
> ```
> --- a/compiler-rt/lib/fuzzer/FuzzerUtil.h
> +++ b/compiler-rt/lib/fuzzer/FuzzerUtil.h
> @@ -106,6 +106,12 @@ inline uint8_t *RoundDownByPage(uint8_t *P) {
>    return reinterpret_cast<uint8_t *>(X);
>  }
>  
> +#if __BYTE_ORDER == __LITTLE_ENDIAN
> +template <typename T> T HToLE(T X) { return X; }
> +#else
> +template <typename T> T HToLE(T X) { return Bswap(X); }
> +#endif
> +
>  }  // namespace fuzzer
>  
>  #endif  // LLVM_FUZZER_UTIL_H
> ```
> 
> ... it's in FuzzerUtil.h and not in FuzzerBuiltins.h, because it uses Bswap, which is defined in a separate header for MSVC.
Sounds good, thanks! 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84913



More information about the llvm-commits mailing list