This is an archive of the discontinued LLVM Phabricator instance.

[bugpoint] Add NoStripSymbols option.
AbandonedPublic

Authored by hintonda on Feb 21 2018, 4:20 PM.

Details

Reviewers
MatzeB
vsk
Summary

Add NoStripSymbols option that matches the behavior of -strip pass.

Much of this patch is a cut-n-paste of several static functions from
StripSymbols.cpp. A better option might be to make them non-static
and add their declarations to a header -- however, since I'm unsure
where they should go, I just cut-n-pasted them for now.

Diff Detail

Event Timeline

hintonda created this revision.Feb 21 2018, 4:20 PM
vsk added a comment.Feb 21 2018, 4:49 PM

A simpler way to do this might be to call 'strip' on the module as an final cleanup, just like we do for GlobalDCE.

In D43601#1015341, @vsk wrote:

A simpler way to do this might be to call 'strip' on the module as an final cleanup, just like we do for GlobalDCE.

So, always run it or use a flag?

vsk added a comment.Feb 21 2018, 4:58 PM
In D43601#1015341, @vsk wrote:

A simpler way to do this might be to call 'strip' on the module as an final cleanup, just like we do for GlobalDCE.

So, always run it or use a flag?

Gating the behavior behind an off-by-default flag sgtm.

In D43601#1015344, @vsk wrote:
In D43601#1015341, @vsk wrote:

A simpler way to do this might be to call 'strip' on the module as an final cleanup, just like we do for GlobalDCE.

So, always run it or use a flag?

Gating the behavior behind an off-by-default flag sgtm.

You mean like the other "disable-strip*" options, right?

It would be unfortunate to have this one behave differently.

But we don't want user flags for bugpoint, we want it to try both variants and keep the smaller one when it still reproduces the crash.

I don't really know the bugpoint code, but I would expect one of those ListReducer thingies getting implemented (like the existing ones in CrashDebugger.cpp) so that bugpoint will try out both: with and without debug info.

Okay reading the code a bit more seems like that is what you are doing.

But we don't want user flags for bugpoint, we want it to try both variants and keep the smaller one when it still reproduces the crash.

Okay reading the code some more, it seems like that is what you are doing here and the option is just to disable the extra try.

  • You should split the OptionCategory changes into a separate commit.
  • I suspect a lot of this code is duplicated with the -strip pass, would it be possible to refactor it into a utility function used by both?
tools/bugpoint/CrashDebugger.cpp
92

maybe this can use a ranged for too? Similar for many of the other for loops.

Okay reading the code a bit more seems like that is what you are doing.

But we don't want user flags for bugpoint, we want it to try both variants and keep the smaller one when it still reproduces the crash.

Okay reading the code some more, it seems like that is what you are doing here and the option is just to disable the extra try.

  • You should split the OptionCategory changes into a separate commit.

Yes, I'll submit that patch separately, and those changes will disappear. I can go ahead and remove them if you want -- just made it easier to see what was going on -- currently the options are just a big ball of mud and hard to grok...

  • I suspect a lot of this code is duplicated with the -strip pass, would it be possible to refactor it into a utility function used by both?

Sure, but first I'm trying to pursue Vedant's suggestion of passing -strip-nondebug to performFinalCleanups(). However, I'm running into an issue I need to debug first. Will report back once I figure that out.

hintonda added inline comments.Feb 21 2018, 7:05 PM
tools/bugpoint/CrashDebugger.cpp
92

Sure, but this is part of the cut-n-paste. Let's see if I can make all of it go away before I start hacking it up...

hintonda updated this revision to Diff 135355.Feb 21 2018, 8:27 PM

Address comments.

In D43601#1015341, @vsk wrote:

A simpler way to do this might be to call 'strip' on the module as an final cleanup, just like we do for GlobalDCE.

Unfortunately this won't work -- we only want -strip-nondebug, but the logic in performFinalCleanups() does a lot more and is predicated on producing output -- which isn't always the case.

MatzeB added inline comments.Feb 22 2018, 4:13 PM
include/llvm/Transforms/IPO.h
34

This needs a doxygen comment.

lib/Transforms/IPO/StripSymbols.cpp
204

You could go for llvm::StripSymbolNames() instead of the namespace llvm {} if it's just this one function (just FYI I don't care either way).

test/BugPoint/strip-names-and-types.ll
17–18

This is also stripping symbols from SSA values and not just from types isn't it? The test should check at least a def and use of an SSA value then I think.

tools/bugpoint/CrashDebugger.cpp
73

This seems superfluous given that it only has a single caller.

hintonda updated this revision to Diff 135562.Feb 22 2018, 4:40 PM
  • Don't clone module when stripping.
  • Address more comments.
hintonda updated this revision to Diff 135567.Feb 22 2018, 5:05 PM
  • Add doxygen comment.
hintonda updated this revision to Diff 135571.Feb 22 2018, 5:21 PM
  • Enhance test.
hintonda abandoned this revision.Mar 24 2018, 9:07 AM

This should only fire for crashes, but the test case is insufficient, and I don't have time right now to try to come up with one.

Also, I've come to believe hacking bugpoint like this is a bad idea -- I prefer replacement.