rework reflection detection with ssa #644
Conversation
|
Generics don't seem to like ssa |
There was a problem hiding this comment.
First review - would like to see perf numbers with our benchmark. I can do that if you're not able to.
It would also be interesting to build std and do a diff of what names are obfuscated, to see how large the difference is, and whether we're now leaving too many unobfuscated names. I think using -debugdir with a fixed seed and diffing the source would be enough, but there might be an easier way like having the old and new versions print how they are obfuscating type declarations.
|
Also, very nice to see that with ~250 lines of ssa code, we're already getting pretty good detection! |
|
There might be a lot more types getting flagged as being in a fmt.Printf call is now enough, but it's right nonetheless. We might need a obfuscate this anyway mechanism similar to what we told the users when our detection didn't work, maybe now it works to well 😂. |
|
I'm not sure whether detecting fmt print calls is something we should do. For example, neither I think we should avoid obfuscating types when it's likely that doing so would break the program. I think that's the case with By the way, please note that in 2ee9cf7 I taught garble to not mark types in the reflect package itself as "used for reflection". You should keep that behavior in place, I think. |
|
Happy to do another review if you fix the conflicts. Thanks again for being patient. |
|
Thanks for your patience. Let's get this reviewed and merged this coming week; I don't want to hold up master for v0.9.x any longer.
I reverted that bit in 5effe20, so I don't think you need to worry about that anymore. Just make sure to break none of the updated tests. If you can resolve the conflicts and reply to my concern about |
This is significantly more robust, than the ast based detection and can record very complex cases of indirect parameter reflection. Fixes burrowers#554
lu4p
force-pushed
the
reflectDetction
branch
from
ea5e789
to
a85687c
Compare
|
|
||
| var reflectSkipPkg = map[string]bool{ | ||
| "fmt": true, | ||
| } |
There was a problem hiding this comment.
why code generate this at all, if it's a static map? in fact, why have a map when you can just do path == "fmt" :)
we might want to expand the logic to also consider other reflect APIs as "harmless" by default, but I don't think we should worry about that for now. I don't think we'll need anything more than just fmt for the time being.
There was a problem hiding this comment.
I like having everything which can't be obfuscated at the same place
| return nil | ||
| } | ||
|
|
||
| /* fmt.Printf("val: %v %T %v\n", val, val, val.Type()) */ |
There was a problem hiding this comment.
nit: use // comments for debugging code, for consistency with the rest of the codebase. same elsewhere.
There was a problem hiding this comment.
I find a visual distinction between comments and debug prints easier to read.
| // methods aren't package members only their reciever types are | ||
| // so some logic is required to find the methods a type has | ||
|
|
||
| // yes, finding all methods really only works with both loops |
There was a problem hiding this comment.
Hmm, that would be surprising to me, and at least misleading documentation in the go/ssa package.
https://go.dev/ref/spec#Method_sets says:
The method set of a pointer to a defined type T (where T is neither a pointer nor an interface) is the set of all methods declared with receiver *T or T.
That is, if you ask for the method set of *T, it should give you both kinds of methods at the same time, avoiding the need for two calls.
I think we should raise a bug upstream (ideally with a small reproducer) and link it here as a TODO.
There was a problem hiding this comment.
Tested it again right now still the case
| if fun.Synthetic == "loaded from gc object file" { | ||
| // fun.WriteTo crashes otherwise | ||
| return | ||
| } |
There was a problem hiding this comment.
this ought to be a TODO; either we need better logic in our code, or we should raise a bug upstream to avoid that crash.
There was a problem hiding this comment.
This is still a problem needs to be fixed upstream
| case *ssa.Slice: | ||
| return tf.recordArgReflected(val.X, visited) | ||
| case *ssa.MakeInterface: | ||
| return tf.recordArgReflected(val.X, visited) | ||
| case *ssa.UnOp: | ||
| return tf.recordArgReflected(val.X, visited) | ||
| case *ssa.FieldAddr: | ||
| return tf.recordArgReflected(val.X, visited) |
There was a problem hiding this comment.
Maybe this can be replaced by .Operands()? It does exactly same thing and eliminates need for type switch
There was a problem hiding this comment.
I know this looks a bit repetitive but .Operands often doesn't do what I'm trying to achieve
This is significantly more robust, than the ast based detection and can record very complex cases of indirect parameter reflection.
Fixes #554