You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
It turns out that between writing the previous fixes and retesting, even more problems arose for java. I think java is more affected because Reftype is bigger than Class, but most uses only need Class so trivial type tests affect java more than other langauges.
List of changes:
The params string change was more important than I thought. Although the fix only removed 10s, that predicate was evaluated multiple times so the saving multiplied. This caches it instead so we only evalaute it in full once.
The changes to AndroidComponent were no longer good enough. Instead this forces the TC to be materialised rather than use the bad magic version (which was done repeatedly). I decided to do this via getAnAncestor so I didn't have to define new predicates and as we evaluate the full TC we always want to use the non-magicked version after that. Note that only one use in each stage needs to be converted but as AndroidComponent was top in the magic as it has the first name alphabetically. I have opened an internal issue about making doing this change easier.
There was fairly bad magic for ConfusingOverloading.ql, the existing magic didn't help much so removing it seems better.
The caching of params string, and the nomagic in ConfusingOverloading.ql LGTM. But I'm not sure I fully understand what goes on with AndroidComponent. This statement in particular is confusing to me:
Note that only one use in each stage needs to be converted but as AndroidComponent was top in the magic as it has the first name alphabetically. I have opened an internal issue about making doing this change easier.
Note also, that the pragma[inline] on getAnAncestor highlights a number of cases where TC is needlessly applied twice resulting in compilation errors - those should be easy fixes.
So with the magic, if within a stage for a call we pick no magic there is no magic ever. This means we simply need a single nomagic call to prevent magic. There were a few other places with problematic magic.
I ended not actually changing getAnAncestor as putting inline on it was problematic due to uses with noopt so I made a more local change.
I have opened an issue about making it easier to prevent magic for tcs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It turns out that between writing the previous fixes and retesting, even more problems arose for java. I think java is more affected because
Reftypeis bigger thanClass, but most uses only needClassso trivial type tests affect java more than other langauges.List of changes:
AndroidComponentwere no longer good enough. Instead this forces the TC to be materialised rather than use the bad magic version (which was done repeatedly). I decided to do this viagetAnAncestorso I didn't have to define new predicates and as we evaluate the full TC we always want to use the non-magicked version after that. Note that only one use in each stage needs to be converted but asAndroidComponentwas top in the magic as it has the first name alphabetically. I have opened an internal issue about making doing this change easier.ConfusingOverloading.ql, the existing magic didn't help much so removing it seems better.