[wip] avoid hardcoded addresses in test cases #294
Conversation
makes this not tied to specific adt files
|
fyi @sadrabt for dsa test changes. but it's not fully done yet :3 |
|
|
||
| # every text except SystemTests: | ||
| # ./mill test.testOnly '*' -- -t '' | tr -d ':' | ansi2txt | grep -vE '^SystemTests' | ||
| - run: ./mill test.testOnly DSAMemoryRegionSystemTestsBAP IntrusiveListTest IntrusiveListPublicInterfaceTest PointsToTest DSAMemoryRegionSystemTestsGTIRB MemoryRegionTestsNoRegion AnalysisSystemTestsBAP AnalysisSystemTestsGTIRB DataStructureAnalysisTest IrreducibleLoop ParamAnalysisTests LiveVarsAnalysisTests IRTest CILVisitorTest InterpreterTests InvariantTest ProcedureSummaryTests UnimplementedTests ExtraSpecTests BitVectorAnalysisTests TaintAnalysisTests IndirectCallTests MemoryRegionTestsDSA |
There was a problem hiding this comment.
We definitely do not want to include all of these in the GitHub action and running all tests simultaneously like this breaks things at present, a bunch of tests (the various SystemTest variations) get in each other's way.
None of the following are expected to pass at present (some are not ever expected to pass) and should not be included:
- MemoryRegionTestsDSA
- MemoryRegionTestsNoRegion
- UnimplementedTests
- ExtraSpecTests
- IndirectCallTests
PointsToTest probably needs to be removed completely, it's very out of date and doesn't work.
IRTest has been bugged for months (#244) and this isn't running it - the tests that are in a particular package need to have the package prefixed like ./mill test.testOnly ir.IRTest
BitVectorAnalysisTests and IntrusiveListPublicInterfaceTest are already in the CompileAndTest job so it's redundant to run them multiple times like this.
There was a problem hiding this comment.
Thank you for the advice, I'll make the changes when I come back :)
|
The addresses should not change as long as the binaries we are lifting from do not change (which the deterministic pipeline should guarantee?), so it should merely be a matter of updating the addresses once for the new test pipeline, rather than needing to try to abstract them like this (though it is also not actually a problem to look up the address of procedures in the program, that's trivial)? The labels seem like the real issue for #288 and I'm not really sure how it's going to be possible to solve that. |
I don't think that's a very good reason against this PR. Previously, I'd been working through some of the address changes on top of #288 and it was extremely tedious to look up each decimal address in the old relf and match it back to change it. Surely, using names in the test cases is more maintainable and descriptive. More importantly, the addresses in the deterministic pipeline are only truly guaranteed if you never change the compiler or source file. I'd like to avoid such restrictions. I agree that the labels are more troublesome since they depend on the lifters. I haven't looked at labels yet, but I did plan to as part of this PR. Anyway, if there are other reasons against this change, then we can talk and see how to address those. Maybe having addresses within the tests is helpful when looking at the relf? Let me know. Happy holidays! (I'll be away from the computer for at least 3 weeks) |
If we change the compiler or C source file then we are changing the test to such a degree that it will need rewriting anyway. I don't think there is any reasonable way t
I don't really have an issue with that, though it's much trickier for the block addresses and I'm not sure what the solution there is. It's just far from really achieving the abstraction you're aiming for which I'm not really sure is possible. |
in preparation for #288, we need to avoid the use of hardcoded addresses in test cases. these addresses can easily be invalidated.
also adds all the unit tests to github actions
todo: