Adding a 'preserve-routine' flag to clang_delta #43

[aytey]

This PR adds a new argument (--preserve-routine=<string>) to clang_delta that allows for replace-function-def-with-decl to replace all routines apart from the one listed in <string>.

It also adds three test-cases to validate this new behaviour.

Signed-off-by: Andrew V. Jones andrewvaughanj@gmail.com

[marxin]

I've just briefly looked at the clang_delta implementation. I would rather suggest skipping the preserved routine both in the code the counts instances, and the code which does transformation (from-to).
Note that C-Vise clang_delta passes are smart as so it first attempts the following transformations for N function:

[1, N]
[1, N/2]
[N/2, N]
[1, N/4]

and so on. Note that all these steps are run in parallel.

[aytey]

Hmm, I wasn't (actually) thinking about this being something that cvise would try to do automatically (and in parallel) -- more of it being an something that a user could decide to do as a very first, initial (potentially manual!) step.

Were you thinking we'd integrate this into the top-level of cvise so someone could specify the preserve-routine at that level?

For my use-case, I would be interested in running clang_delta (manually), removing all but the body of this one routine, and then running cvise normally (which then would reduce this routine!). In this workflow, I don't need cvise to be aware of the preserve-routine option.

Given this:

• Are you still okay with the feature?
• What are your thoughts on this being "oblivious" to cvise? If you'd like cvise to be aware of it, how do you think that interaction would look?

[marxin]

Hmm, I wasn't (actually) thinking about this being something that cvise would try to do automatically (and in parallel) -- more of it being an something that a user could decide to do as a very first, initial (potentially manual!) step.

Well, I would not do a manual step as replace-function-def-with-decl runs very early in the passes:

00:00:00 INFO INITIAL PASSES
00:00:00 INFO ===< IncludesPass >===
00:00:00 INFO ===< UnIfDefPass >===
00:00:00 INFO ===< CommentsPass >===
00:00:00 INFO ===< IfPass >===
00:00:00 INFO ===< LineMarkersPass >===
00:00:00 INFO ===< BlankPass >===
00:00:00 INFO ===< ClangBinarySearchPass::replace-function-def-with-decl >===

If you want to start with ClangBinarySearchPass::replace-function-def-with-decl, you can use cvise --start-with-pass=ClangBinarySearchPass::replace-function-def-with-decl.

I've added cvise option for that and pushed that here:
https://github.com/marxin/cvise/tree/preserve_routine

If you want to run it manually, it's fine. You will need to add --counter=1 --to-counter=999999999 --warn-on-counter-out-of-bounds and it will do what you want.

Were you thinking we'd integrate this into the top-level of cvise so someone could specify the preserve-routine at that level?

For my use-case, I would be interested in running clang_delta (manually), removing all but the body of this one routine, and then running cvise normally (which then would reduce this routine!). In this workflow, I don't need cvise to be aware of the preserve-routine option.

As mentioned, it seems to me as an extra step.
Are you fine with the updated pull request I've made in the branch?

Given this:

• Are you still okay with the feature?
• What are your thoughts on this being "oblivious" to cvise? If you'd like cvise to be aware of it, how do you think that interaction would look?

[aytey]

Your changes look great to me, and I have no issues with needing to specify a "from" and "to" value to get this to work!

I think it is a good change that if you query and provide a preserve option, that you now get the correct count (vs. my change where you'd still get all the instances).

[codecov-io]

Codecov Report

Merging #44 (5ad2b3d) into master (0b50c0a) will increase coverage by 0.06%.
The diff coverage is 65.21%.

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
+ Coverage   40.53%   40.59%   +0.06%     
==========================================
  Files         179      179              
  Lines       16309    16331      +22     
  Branches     2068     2070       +2     
==========================================
+ Hits         6611     6630      +19     
- Misses       9104     9106       +2     
- Partials      594      595       +1     

Impacted Files	Coverage Δ
clang_delta/Transformation.h	0.00% <ø> (ø)
clang_delta/TransformationManager.h	100.00% <ø> (ø)
cvise/passes/clangbinarysearch.py	61.25% <25.00%> (-1.91%)	⬇️
clang_delta/ClangDelta.cpp	35.09% <33.33%> (-0.08%)	⬇️
clang_delta/ReplaceFunctionDefWithDecl.cpp	28.12% <50.00%> (+3.44%)	⬆️
clang_delta/TransformationManager.cpp	56.86% <100.00%> (+0.42%)	⬆️
clang_delta/tests/test_clang_delta.py	99.71% <100.00%> (+<0.01%)	⬆️
cvise.py	78.09% <100.00%> (+0.10%)	⬆️
cvise/cvise.py	88.98% <100.00%> (+0.09%)	⬆️
clex/strlex.c
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b50c0a...5ad2b3d. Read the comment docs.

[marxin]

Great, let's merge it now.
Thank you for your cooperation.

[marxin]

I have one nit: would you be able to preserve a routine that can have a complex template name?

Like for:
https://github.com/gcc-mirror/gcc/blob/master/gcc/testsuite/g++.dg/gcov/pr88045.C

c++filt _ZSt4moveIRNSt12_Vector_baseI4TreeIJ3FooS2_EESaIS3_EE12_Vector_implEEONSt16remove_referenceIT_E4typeEOS9_
std::remove_reference<std::_Vector_base<Tree<Foo, Foo>, std::allocator<Tree<Foo, Foo> > >::_Vector_impl&>::type&& std::move<std::_Vector_base<Tree<Foo, Foo>, std::allocator<Tree<Foo, Foo> > >::_Vector_impl&>(std::_Vector_base<Tree<Foo, Foo>, std::allocator<Tree<Foo, Foo> > >::_Vector_impl&)

?

[aytey]

Hmm, yes, you've got a good point. While mostly this won't be an issue, I can see why it might be nice to have this functionality.

I played around, and couldn't find a good way to get either the "full" name or the mangled name from the LLVM API. If we know how to get that "full" name, then I'm totally fine to need to give that name.

Currently, for pr88045.C, we have these names:

Foo::size
Foo::Foo
Tree::Tree<Ts...>
Tree::Tree<Ts...>
size
size
main

which isn't great, but also isn't entirely horrible ...

[marxin]

I guess there must be a way how to get a mangled name from LLVM API. Anyway, we may want to leave it for the future.
I can also imagine a regex that can cover function names that should be preserved.

[aytey]

Interestingly, LLVM even has its own in-built regex class:

• https://llvm.org/doxygen/classllvm_1_1Regex.html

I think the idea about getting the fully qualified names + using a regex sounds great. Personally, I would rather have the unmangled name (despite being more verbose), as I think that will be easier for a user.

We may also want some way to use clang_delta to also dump all of the routine names, so a user can see the precise syntax that clang_delta wants.

Shall I create a GitHub issue around these and I can take a look if I get some time?

[marxin]

We may also want some way to use clang_delta to also dump all of the routine names, so a user can see the precise syntax that clang_delta wants.

Shall I create a GitHub issue around these and I can take a look if I get some time?

Yes, that should be a useful feature for the newly added --preserve-routine, thanks.