[libFuzzer] add attribute noinline on Fuzzer::ExecuteCallback()

[jonpa]

Summary

The inlining of this function caused minimize_two_crashes.test to fail on a stage-2 (clang) build, so it is necessary to disable it.

The reason the test fails is that during stack unwinding it expects to find this function, get a string representation that matches it, like:

DedupToken1: DEDUP_TOKEN: Bar()--LLVMFuzzerTestOneInput--fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long)
DedupToken2: DEDUP_TOKEN: Bar()--LLVMFuzzerTestOneInput--fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long)

When inlined the strings do not match anymore, and the test fails:

DedupToken1: DEDUP_TOKEN: Bar()--LLVMFuzzerTestOneInput--fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long)
DedupToken2: DEDUP_TOKEN: Bar()--LLVMFuzzerTestOneInput--ExecuteCallback
mismatch in dedup tokens (looks like a different bug). Won't minimize further

It may be possible to in the future fix the clang symbolization of the function signature of the inlined function to match the expected full string, like happens with a gcc build. The noinline attribute can then be removed again, even though the function should be large enough to not demand inlining for performance reasons.

This is intended to fix https://bugs.llvm.org/show_bug.cgi?id=49152.

[aeubanks]

it's symbolization+inlining on a specific platform that doesn't 100% work, right? are there any existing test cases or bugs that show this?

there should probably be a FIXME explaining why this noinline was added.

why does the inlining only affect one of the two symbolizations?

the description should probably reference PR49152.

[jonpa]

it's symbolization+inlining on a specific platform that doesn't 100% work, right? are there any existing test cases or bugs that show this?

Yes, that's how it seems. I don't know if there are any tests or bugs reported for this, but I am not aware of any.

there should probably be a FIXME explaining why this noinline was added.

OK, I added a comment in the patch.

why does the inlining only affect one of the two symbolizations?

These are the two commands executed (matching DedupToken1/2):

CRASH_MIN: executing: /home/ijonpan/llvm-project/stage2/projects/compiler-rt/test/fuzzer/S390XDefaultLinuxConfig/Output/minimize_two_crashes.test.tmp-TwoDifferentBugsTest -seed=1 -max_total_time=600 /home/ijonpan/llvm-project/stage2/projects/compiler-rt/test/fuzzer/S390XDefaultLinuxConfig/Output/minimize_two_crashes.test.tmp/long_crash 2>&1
CRASH_MIN: executing: /home/ijonpan/llvm-project/stage2/projects/compiler-rt/test/fuzzer/S390XDefaultLinuxConfig/Output/minimize_two_crashes.test.tmp-TwoDifferentBugsTest -seed=1 -max_total_time=600 /home/ijonpan/llvm-project/stage2/projects/compiler-rt/test/fuzzer/S390XDefaultLinuxConfig/Output/minimize_two_crashes.test.tmp/long_crash -minimize_crash_internal_step=1 -exact_artifact_path=/home/ijonpan/llvm-project/stage2/projects/compiler-rt/test/fuzzer/S390XDefaultLinuxConfig/Output/minimize_two_crashes.test.tmp/result 2>&1n

The second one is the same but adds '-minimize_crash_internal_step=1 -exact_artifact_path=/home/ijonpan/llvm-project/stage2/projects/compiler-rt/test/fuzzer/S390XDefaultLinuxConfig/Output/minimize_two_crashes.test.tmp/result'.

In both of these the segv in the test case triggers the FuzzerUtilPosix.cpp::SegvHandler(), but I am not sure exactly what else could be the difference. It's quite difficult for me to figure out through gdb so maybe someone more knowledgeable could give an explanation?

the description should probably reference PR49152.

done

[jonpa]

clang-format

[aeubanks]

IMO it seems better to disable the test on s390x rather than to add this noinline attribute in the sources. WDYT?

[morehouse]

In D97975#2611534, @aeubanks wrote:

IMO it seems better to disable the test on s390x rather than to add this noinline attribute in the sources. WDYT?

I agree.

[jonpa]

In D97975#2611534, @aeubanks wrote:

IMO it seems better to disable the test on s390x rather than to add this noinline attribute in the sources. WDYT?

Everybody OK with that?

[uweigand]

I don't think there is a s390x platform-specific problem involved here at all, so I'm not sure disabling the test for the platform is the right fix.

What seems to happen is:

• The test case intentionally compares the stack traces from two different invocations:

(1) RunOneTest->ExecuteCallback->LLVMFuzzerTestOneInput->Bar
(2) MinimizeCrashLoop->ExecuteCallback->LLVMFuzzerTestOneInput->Bar

• It just so happens that on s390x under these particular circumstances, the ExecuteCallback routine is *not* inlined into RunOneTest in (1), but it *is* inlined into MinimizeCrashLoop in (2). This is the only platform-specific aspect of the whole scenario, and it does not indicate any bug or problem; it could happen elsewhere too.
• The llvm-symbolizer will retrieve the function name for normal (non-inlined) frames from the symbol table, but the function name for an inline frame from debug info.
• For C++ code, names found in the symbol table are mangled, and are printed in demangled form including the function prototype. However, names found in debug info are just plain unmangled names, and are printed as such (without prototype). This means that there is a difference in the printed output between inlined and non-inlined frames. This difference causes the fuzzer test to fail.

In theory the symbolizer can probably be improved to also print the prototype for inlined C++ functions -- the argument type information is available in debug info too, it's just not encoded into the mangled name but rather into DWARF debug statements. Those could also be decoded, but that would need to be implemented. But I don't think this is really what this test is all about anyway.

Given the current symbolizer behavior, this test case implicitly assumes that the invocations of ExecuteCallback in (1) and (2) above are either both inlined or none of them are. Therefore, I think Jonas' patch makes sense here, in that it ensure that this assumption is always met.

[morehouse]

LGTM

[aeubanks]

Ah I see, that makes sense. The comment should be updated to not be a FIXME in that case.

[morehouse]

This broke the windows build: https://lab.llvm.org/buildbot/#/builders/127/builds/7433

C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\lib\fuzzer\FuzzerLoop.cpp(583): error C2065: 'noinline': undeclared identifier
C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\lib\fuzzer\FuzzerLoop.cpp(583): error C2182: '__attribute__': illegal use of type 'void'
C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\lib\fuzzer\FuzzerLoop.cpp(584): error C2143: syntax error: missing ';' before 'fuzzer::Fuzzer::ExecuteCallback'
C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\lib\fuzzer\FuzzerLoop.cpp(584): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int
C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\lib\fuzzer\FuzzerLoop.cpp(584): error C2556: 'int fuzzer::Fuzzer::ExecuteCallback(const uint8_t *,size_t)': overloaded function differs only by return type from 'void fuzzer::Fuzzer::ExecuteCallback(const uint8_t *,size_t)'
c:\b\slave\sanitizer-windows\llvm-project\compiler-rt\lib\fuzzer\FuzzerInternal.h(68): note: see declaration of 'fuzzer::Fuzzer::ExecuteCallback'
C:\b\slave\sanitizer-windows\llvm-project\compiler-rt\lib\fuzzer\FuzzerLoop.cpp(584): error C2371: 'fuzzer::Fuzzer::ExecuteCallback': redefinition; different basic types
c:\b\slave\sanitizer-windows\llvm-project\compiler-rt\lib\fuzzer\FuzzerInternal.h(68): note: see declaration of 'fuzzer::Fuzzer::ExecuteCallback'

[morehouse]

In D97975#2622936, @morehouse wrote:

This broke the windows build: https://lab.llvm.org/buildbot/#/builders/127/builds/7433

Fixed in: https://reviews.llvm.org/rG4b82f614745b

compiler-rt/lib/fuzzer/FuzzerLoop.cpp

static bool LooseMemeq(const uint8_t *A, const uint8_t *B, size_t Size) {
   const size_t Limit = 64;
   if (Size <= 64)
     return !memcmp(A, B, Size);
   // Compare first and last Limit/2 bytes.
   return !memcmp(A, B, Limit / 2) &&
          !memcmp(A + Size - Limit / 2, B + Size - Limit / 2, Limit / 2);
 }
 
-void Fuzzer::ExecuteCallback(const uint8_t *Data, size_t Size) {
+// This method is not inlined because it would cause a test to fail where it
+// is part of the stack unwinding. See D97975 for details.
+void __attribute__((noinline))
+Fuzzer::ExecuteCallback(const uint8_t *Data, size_t Size) {
   TPC.RecordInitialStack();
   TotalNumberOfRuns++;
   assert(InFuzzingThread());
   // We copy the contents of Unit into a separate heap buffer
   // so that we reliably find buffer overflows in it.
   uint8_t *DataCopy = new uint8_t[Size];
   memcpy(DataCopy, Data, Size);
   if (EF->__msan_unpoison)