Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix shape inference for math operators #50

Merged
merged 1 commit into from
May 17, 2019

Conversation

EliaGeretto
Copy link
Contributor

Supposing to have a label lb, corresponding to the bit vector 001111, the path describing it on the tree could be as follows, with lb pointing to the node that contains [5,6).

     [0,0)
     /
  [0,2)
   / \
...  [2,3)
        \
        [3,4)
           \
           [4,5)
              \
              [5,6)

Calling infer_shape2(lb, 4), the function should be able to group bytes from 2 to 5 together. This, however, with the current implementation, does not happen. After some debugging, it appears that the for loop is moving the cur_lb one too many times and make it point to the root, instead of [2, 3), when terminating the loop. This patch fixes that behavior and correctly groups the bytes, producing the following tree after the execution:

     [0,0)
     /
  [0,2)
   / \
...  [2,6)
        \
        [3,4)
           \
           [4,5)
              \
              [5,6)

If this is not the intended behavior, please let me know.

@EliaGeretto
Copy link
Contributor Author

My doubt on the expected behavior stems from the fact that using the infer_shape function to merge those four bytes together, the resulting tree would be the following:

     [0,0)
     /
  [0,2)
   / \
...  [2,3)
        \
        [3,4)
           \
           [4,5)
              \
              [2,6)

As you can see, the node that gets modified is the one at the bottom and not the one at the top.

@spinpx spinpx changed the base branch from llvm7 to dev May 17, 2019 06:50
@spinpx
Copy link
Member

spinpx commented May 17, 2019

Thanks for your feedback, EliaGeretto. It is not intended behaviors. I have merged it to dev branch and will merge to master soon.

@spinpx spinpx merged commit 871b880 into AngoraFuzzer:dev May 17, 2019
@EliaGeretto
Copy link
Contributor Author

Thank you for your clarification! However, there is still a question that you did not answered. Is the difference between shape inference for math operators and loads intended? The difference between the second and the third tree I posted here. In the first, the aggregation node is at the beginning of the sequence, in the second one, it is at the end.

If you want to make the behavior uniform, I can create another pull request. It's a single line change within the same function.

@spinpx
Copy link
Member

spinpx commented May 17, 2019

You are right. They had better should be consistent. I fix this issue, and add tests in 66582b3 .

Pleasse check it.
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants