[libfuzzer] do not use xargs for shell expansion

[aizatsky]

Summary

After https://reviews.llvm.org/rL296904 xargs shouldn't be necesssary.

[kcc]

that's not any better, and probably worse:

• still have xargs
• now also depends on find which on windows may be different

[mehdi_amini]

At least the usage of xargs seems correct now!

This works on macOS.

[zturner]

Given that this problem keeps happening and is not likely to go away any time soon, I wonder if the best solution is to build some special syntax into lit that can expand globs. lit already has plenty of cases where it does text replacements, for example if you write %t in a run line, or {{.*}} in a match line.

Could we add some sort of replacement sequence that can be used in run lines which expands a glob expression? One idea for a possible syntax might be %[expression], where expression can itself be another substituion sequence. This would allow you to write line 3 above as:

RUN: sancov -covered-functions %[LLVMFuzzer-NullDerefTest*] %[%t_workdir/*.sancov] | FileCheck %s --check-prefix=SANCOV

[mehdi_amini]

In D30371#686712, @zturner wrote:

Given that this problem keeps happening and is not likely to go away any time soon, I wonder if the best solution is to build some special syntax into lit that can expand globs. lit already has plenty of cases where it does text replacements, for example if you write %t in a run line, or {{.*}} in a match line.

Nit: {{.*}} is a FileCheck expansion, not a lit one (unless I misunderstood what you mean).

Could we add some sort of replacement sequence that can be used in run lines which expands a glob expression? One idea for a possible syntax might be %[expression], where expression can itself be another substituion sequence. This would allow you to write line 3 above as:

RUN: sancov -covered-functions %[LLVMFuzzer-NullDerefTest*] %[%t_workdir/*.sancov] | FileCheck %s --check-prefix=SANCOV

I'm not sure I understand the first expansion, but I'm fine with the second one!

[mehdi_amini]

In the meantime, this seems like a strict improvement to me.

[zturner]

In D30371#686713, @mehdi_amini wrote:

In D30371#686712, @zturner wrote:

Given that this problem keeps happening and is not likely to go away any time soon, I wonder if the best solution is to build some special syntax into lit that can expand globs. lit already has plenty of cases where it does text replacements, for example if you write %t in a run line, or {{.*}} in a match line.

Nit: {{.*}} is a FileCheck expansion, not a lit one (unless I misunderstood what you mean).

Could we add some sort of replacement sequence that can be used in run lines which expands a glob expression? One idea for a possible syntax might be %[expression], where expression can itself be another substituion sequence. This would allow you to write line 3 above as:

RUN: sancov -covered-functions %[LLVMFuzzer-NullDerefTest*] %[%t_workdir/*.sancov] | FileCheck %s --check-prefix=SANCOV

I'm not sure I understand the first expansion, but I'm fine with the second one!

I was looking at the left side of the test (i.e. before the change). It writes LLVMFuzzer-NullDerefTest*, I'm not sure why the left side has a * and the right side doesn't though. Seems like an oversight in this patch (I don't know this code well, just that the left and right don't seem equivalent due to the loss of the * on the right hand side. So the equivalent expansion for the left hand side would be %[LLVMFuzzer-NullDerefTest*]

[mehdi_amini]

Oh right!

I don't know what the intent was but this is the name of the binary, so it is likely that it use to expand to itself only?

[aizatsky]

In D30371#686718, @mehdi_amini wrote:

Oh right!

I don't know what the intent was but this is the name of the binary, so it is likely that it use to expand to itself only?

In this case LLVMFuzzer-NullDerefTest* is not necessary.

[kcc]

Remember about windows, where LLVMFuzzer-NullDerefTest* expands to LLVMFuzzer-NullDerefTest.exe

[zturner]

I've attempted to solve this in D30380 btw. Take a look and LMK if you think the solution proposed there is desirable.

[zturner]

My patch has been committed. Can you try it now by writing:

RUN: sancov -covered-functions LLVMFuzzer-NullDerefTest%exeext %t_workdir/*.sancov | FileCheck %s --check-prefix=SANCOV

[aizatsky]

removed xargs

[aizatsky]

The test passes on mac without xargs. Could you verify windows?

[rnk]

In D30371#692007, @aizatsky wrote:

The test passes on mac without xargs. Could you verify windows?

Sure, it works now.

[kubamracek]

The test now seems to fail on macOS, http://green.lab.llvm.org/green/view/Experimental/job/libFuzzer/477/console. Could you take a look?

[mehdi_amini]

The test was XFAIL, since this patch fixes it, it is now "failing" because it passes ;)

XPASS: LLVMFuzzer :: dump_coverage.test (34 of 81)

[kubamracek]

Oh, right! I'll submit a fix to un-XFAIL it.

[kubamracek]

Un-XFAIL'd in r297110.

llvm/trunk/lib/Fuzzer/test/dump_coverage.test

 RUN: rm -rf %t_workdir && mkdir -p %t_workdir
 RUN: env ASAN_OPTIONS=coverage_dir='"%t_workdir"' not LLVMFuzzer-NullDerefTest -dump_coverage=1 2>&1 | FileCheck %s
-RUN: xargs sancov -covered-functions LLVMFuzzer-NullDerefTest* %t_workdir/*.sancov | FileCheck %s --check-prefix=SANCOV
+RUN: sancov -covered-functions LLVMFuzzer-NullDerefTest* %t_workdir/*.sancov | FileCheck %s --check-prefix=SANCOV
 RUN: env ASAN_OPTIONS=coverage_dir='"%t_workdir"' LLVMFuzzer-DSOTest -dump_coverage=1 -runs=0 2>&1 | FileCheck %s --check-prefix=DSO
 RUN: env ASAN_OPTIONS=coverage_dir='"%t_workdir"' not LLVMFuzzer-NullDerefTest -dump_coverage=0 2>&1 | FileCheck %s --check-prefix=NOCOV
 
 XFAIL: darwin
 
 CHECK: SanitizerCoverage: {{.*}}LLVMFuzzer-NullDerefTest.{{.*}}.sancov {{.*}} PCs written
 SANCOV: LLVMFuzzerTestOneInput
 
 DSO: SanitizerCoverage: {{.*}}LLVMFuzzer-DSOTest.{{.*}}.sancov {{.*}} PCs written
 DSO-DAG: SanitizerCoverage: {{.*}}LLVMFuzzer-DSO1.{{.*}}.sancov {{.*}} PCs written
 DSO-DAG: SanitizerCoverage: {{.*}}LLVMFuzzer-DSO2.{{.*}}.sancov {{.*}} PCs written
 
 NOCOV-NOT: SanitizerCoverage: {{.*}} PCs written