This is an archive of the discontinued LLVM Phabricator instance.

[libFuzzer] Enable for SystemZ
ClosedPublic

Authored by iii on Jul 30 2020, 12:51 AM.

Details

Summary
  • Add SystemZ to the list of supported architectures.
  • Fix endianness issue in ForEachNonZeroByte().
  • Fix types of stop* and start* symbols. Adapt tests.

If a section is supposed to hold elements of type T, then the
corresponding CreateSecStartEnd()'s Ty parameter represents T*.
Forwarding it to GlobalVariable constructor causes the resulting
GlobalVariable's type to be T*, and its SSA value type to be T**, which
is one indirection too many. This issue is mostly masked by pointer
casts, however, the global variable still gets an incorrect alignment,
which causes SystemZ to choose wrong instructions to access the
section.

  • XFAIL a few tests.

Coverage reporting is broken, and is not easy to fix (see comment in
coverage.test). Interaction with sanitizers needs to be investigated
more thoroughly, since they appear to reduce coverage in certain cases.

Diff Detail

Event Timeline

iii created this revision.Jul 30 2020, 12:51 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 30 2020, 12:51 AM
Herald added subscribers: llvm-commits, Restricted Project, hiraditya, mgorny. · View Herald Transcript
iii requested review of this revision.Jul 30 2020, 12:51 AM
iii updated this revision to Diff 281818.

Fixed Reviewers: tag.

iii edited the summary of this revision. (Show Details)Jul 30 2020, 12:53 AM
iii added reviewers: kcc, morehouse, uweigand, jonpa.
Kai added a subscriber: Kai.Jul 30 2020, 2:05 AM
kcc added a comment.Jul 30 2020, 7:57 AM

The compiler change seems to be completely independent from the libFuzzer change.
Please split this change into two.

compiler-rt/lib/fuzzer/FuzzerTracePC.h
198

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?

iii added a comment.Jul 30 2020, 9:12 AM

Ok, I'll split the patch.

compiler-rt/lib/fuzzer/FuzzerTracePC.h
198

I think this might not work as expected on 32-bit systems. I'll add a separate function.

kcc added inline comments.Jul 30 2020, 9:20 AM
compiler-rt/lib/fuzzer/FuzzerTracePC.h
198

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.

iii added inline comments.Jul 30 2020, 9:28 AM
compiler-rt/lib/fuzzer/FuzzerTracePC.h
198

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.

kcc added inline comments.Jul 30 2020, 9:43 AM
compiler-rt/lib/fuzzer/FuzzerTracePC.h
198

Sounds good, thanks!

iii updated this revision to Diff 281979.Jul 30 2020, 10:48 AM

Split in three commits, removed #ifdef from ForEachNonZeroByte() body.

iii updated this revision to Diff 281990.Jul 30 2020, 10:55 AM

Second attempt to push all 3 commits with arcanist. Sorry for the noise
if it doesn't work out :-/

iii updated this revision to Diff 281991.Jul 30 2020, 11:03 AM

arc which origin/master tells me that arc diff origin/master will
send all the 3 commits. Fingers crossed.

iii updated this revision to Diff 281993.EditedJul 30 2020, 11:12 AM
  • [libFuzzer] Fix endianness issue in ForEachNonZeroByte()
  • [libFuzzer] Fix types of stop* and start* symbols
  • [libFuzzer] Enable for SystemZ

Somehow having Differential Revision: in commit descriptions prevented arcanist from sending all commits. Removing it helped.

iii updated this revision to Diff 282046.Jul 30 2020, 2:19 PM

Two minor fixes:

  • Remove unneeded <endian.h> include.
  • Make clang-tidy happy by renaming HToLE to Htole.
iii updated this revision to Diff 282179.Jul 31 2020, 3:48 AM
  • Htole -> htole (I missed clang-tidy suggestion)
  • [libFuzzer] -> [SanitizerCoverage] in the 2nd patch title
iii added a comment.Aug 4 2020, 10:20 AM

@kcc Could you have another look please?

kcc accepted this revision.Aug 4 2020, 10:31 AM

Please fix two nits, then good to go.
Thanks!

compiler-rt/lib/fuzzer/FuzzerTracePC.h
196–198

please remove the { that weren't there.

compiler-rt/lib/fuzzer/FuzzerUtil.h
110

Maybe name it HostToLE?

This revision is now accepted and ready to land.Aug 4 2020, 10:31 AM
iii updated this revision to Diff 282973.Aug 4 2020, 11:27 AM
  • Remove unnecessary {}
  • htole -> HostToLE
iii marked 5 inline comments as done.Aug 4 2020, 11:28 AM
iii added inline comments.
compiler-rt/lib/fuzzer/FuzzerTracePC.h
196–198

ok

compiler-rt/lib/fuzzer/FuzzerUtil.h
110

Ok - clang-tidy might not like it, but let's see.

iii marked 2 inline comments as done.Aug 4 2020, 12:39 PM
iii added inline comments.
compiler-rt/lib/fuzzer/FuzzerUtil.h
110

Yeah: invalid case style for function 'HostToLE'. Suggested name is 'hostToLe'.
Would it be ok to commit with this warning?

kcc added inline comments.Aug 4 2020, 12:45 PM
compiler-rt/lib/fuzzer/FuzzerUtil.h
110

I don't mind.
hostToLe is fine too.

iii closed this revision.Aug 4 2020, 1:00 PM
iii marked an inline comment as done.

I've committed with HostToLE, since it matches the existing code style.

Thanks for the review!