Page MenuHomePhabricator

[libfuzzer] do not use xargs for shell expansion
ClosedPublic

Authored by aizatsky on Feb 24 2017, 8:18 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

aizatsky created this revision.Feb 24 2017, 8:18 PM
aizatsky added a project: Restricted Project.
aizatsky added a subscriber: llvm-commits.
kcc edited edge metadata.Feb 24 2017, 8:28 PM

that's not any better, and probably worse:

  • still have xargs
  • now also depends on find which on windows may be different
mehdi_amini edited edge metadata.Feb 24 2017, 8:53 PM

At least the usage of xargs seems correct now!

This works on macOS.

zturner edited edge metadata.Feb 25 2017, 11:41 AM

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

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 accepted this revision.Feb 25 2017, 11:49 AM

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

This revision is now accepted and ready to land.Feb 25 2017, 11:49 AM

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*]

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?

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 added a comment.Feb 27 2017, 12:34 PM

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

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

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 updated this revision to Diff 90534.Mar 3 2017, 2:35 PM

removed xargs

aizatsky retitled this revision from [libfuzzer] use find to feed xargs to [libfuzzer] do not use xargs for shell expansion.Mar 3 2017, 2:35 PM
aizatsky edited the summary of this revision. (Show Details)

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

rnk added a subscriber: rnk.Mar 3 2017, 3:33 PM

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

Sure, it works now.

This revision was automatically updated to reflect the committed changes.

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

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

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

Un-XFAIL'd in r297110.