Page MenuHomePhabricator

Revert "SimplifyCFG: Clean up optforfuzzing implementation"
AbandonedPublic

Authored by zequanwu on Oct 21 2020, 5:14 PM.

Details

Summary

In commit cdd006eec9409923f9a56b9026ce2cb72e7b71dc, the options SimplifyCondBranch and FoldTwoEntryPHINode were added and set to false when attribute OptForFuzzing is present. We also need to do the same for SimplifyCFGPass which is NPM version of CFGSimplifyPass. It will be duplicated. We can just check for the attribute at simplifyCondBranch and FoldTwoEntryPHINode
See discussion: https://reviews.llvm.org/D89590
This reverts commit cdd006eec9409923f9a56b9026ce2cb72e7b71dc.

Diff Detail

Unit TestsFailed

TimeTest
360 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
110 mswindows > LLVM.tools/llvm-symbolizer/pdb::missing_pdb.test
Script: -- : 'RUN: at line 1'; not c:\ws\w64\llvm-project\premerge-checks\build\bin\llvm-symbolizer.exe 0x401000 0x401001 -obj="C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-symbolizer\pdb/Inputs/missing_pdb.exe" 2>C:\ws\w64\llvm-project\premerge-checks\build\test\tools\llvm-symbolizer\pdb\Output\missing_pdb.test.tmp.err | c:\ws\w64\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w64\llvm-project\premerge-checks\llvm\test\tools\llvm-symbolizer\pdb\missing_pdb.test

Event Timeline

zequanwu created this revision.Oct 21 2020, 5:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 21 2020, 5:14 PM
zequanwu requested review of this revision.Oct 21 2020, 5:14 PM

lgtm, but I'll let arsenm approve

arsenm requested changes to this revision.Oct 21 2020, 5:35 PM

I disagree with this change. SimplifyCFG should continue to pass in options for transform options as is done for the others. I don't think avoiding duplication between the new PM and code that will be deleted is worth it

This revision now requires changes to proceed.Oct 21 2020, 5:35 PM

https://reviews.llvm.org/D44232 makes it sound like we always want to disable certain optimizations when optforfuzzing is specified, so the checks should be done in the transform, not the pass setup. We can have the options you introduced to disable those optimizations no matter what (as I suggested in https://reviews.llvm.org/D89590), but I think we should at least always check for optforfuzzing in SimplifyCFG.cpp. Does that make sense?

lebedev.ri accepted this revision.Oct 21 2020, 11:15 PM
lebedev.ri added a subscriber: lebedev.ri.

Thank you for the patch!

I disagree with this change. SimplifyCFG should continue to pass in options for transform options as is done for the others. I don't think avoiding duplication between the new PM and code that will be deleted is worth it

I disagree here. Why is that beneficial, if we want to force-disable those transforms regardless of their previous configured status?

Thank you for the patch!

I disagree with this change. SimplifyCFG should continue to pass in options for transform options as is done for the others. I don't think avoiding duplication between the new PM and code that will be deleted is worth it

I disagree here. Why is that beneficial, if we want to force-disable those transforms regardless of their previous configured status?

Because SimplifyCFG is a standalone utility, not just a pass. It's a pass's responsibility to ensure attributes are respected, not individual transformations. If I'm writing another control flow pass (e.g. a structurizer) and calling these utilities, the transformation options should not be limited based on this extremely poorly specified attribute.

lebedev.ri added a comment.EditedOct 22 2020, 6:49 AM

Thank you for the patch!

I disagree with this change. SimplifyCFG should continue to pass in options for transform options as is done for the others. I don't think avoiding duplication between the new PM and code that will be deleted is worth it

I disagree here. Why is that beneficial, if we want to force-disable those transforms regardless of their previous configured status?

Because SimplifyCFG is a standalone utility, not just a pass. It's a pass's responsibility to ensure attributes are respected, not individual transformations. If I'm writing another control flow pass (e.g. a structurizer) and calling these utilities, the transformation options should not be limited based on this extremely poorly specified attribute.

Let's back-track here.
Are you saying that those folds should not disable themselves in presence of OptForFuzzing attribute,
or that they should be guarded by their options?
Because i agree with later, but disagree with former.

Thank you for the patch!

I disagree with this change. SimplifyCFG should continue to pass in options for transform options as is done for the others. I don't think avoiding duplication between the new PM and code that will be deleted is worth it

I disagree here. Why is that beneficial, if we want to force-disable those transforms regardless of their previous configured status?

Because SimplifyCFG is a standalone utility, not just a pass. It's a pass's responsibility to ensure attributes are respected, not individual transformations. If I'm writing another control flow pass (e.g. a structurizer) and calling these utilities, the transformation options should not be limited based on this extremely poorly specified attribute.

Let's back-track here.
Are you saying that those folds should not disable themselves in presence of OptForFuzzing attribute,
or that they should be guarded by their options?
Because i agree with later, but disagree with former.

I'm saying these folds should be guarded by the SimplifyCFGOptions flags. The pass itself should initialize the option field based on the attribute. Other uses of SimplifyCFG the utilities can consider the attribute at their discretion

+morehouse who added optforfuzzing

morehouse, any thoughts on this?

I'm not familiar with users of standalone SimplifyCFG.

My primary concern is to keep these optimizations disabled under -fsanitize=fuzzer since they reduce the coverage signal we get from SanCov. AFAIU, either approach works for this.

zequanwu abandoned this revision.Nov 17 2020, 2:37 PM