-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
coverage: Get coverage of the various code emitters. #18002
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
base: master
Are you sure you want to change the base?
Conversation
Code size report:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This is necessary so that the coverage port can generate code for other CPUs but still run its own tests with native code. Signed-off-by: Jeff Epler <jepler@gmail.com>
Otherwise, when multiple emitters are enabled, the wrong one may be chosen. Signed-off-by: Jeff Epler <jepler@gmail.com>
The preprocessor block for selecting the correct native arch needs checking. The old version assumed only a single emitter was ever enabled, and that it would match the executing CPU. I added further |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18002 +/- ##
==========================================
- Coverage 98.38% 96.74% -1.65%
==========================================
Files 171 184 +13
Lines 22296 25013 +2717
==========================================
+ Hits 21937 24199 +2262
- Misses 359 814 +455 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There are some sanitizer finds in the tests (ubsan), as I had "hoped":
The PR achieves e.g., 89% of coverage in asmthumb.c in contrast to no coverage "before". |
4c2ce17
to
e3ca7e7
Compare
"emitinline" should also now get coverage. |
.. with supporting changes across the core. Signed-off-by: Jeff Epler <jepler@gmail.com>
Signed-off-by: Jeff Epler <jepler@gmail.com>
Ths fixes the following type of diagnostic: "runtime error: left shift of 14 by 28 places cannot be represented in type 'int'" and ensures the resulting 32-bit value is correct. Signed-off-by: Jeff Epler <jepler@gmail.com>
This fixes the diagnostic "runtime error: left shift of negative value -1" and ensures the correct instruction is assembled. Signed-off-by: Jeff Epler <jepler@gmail.com>
Signed-off-by: Jeff Epler <jepler@gmail.com>
Summary
I identified that the various code emitters were not well covered during CI (in terms of codecov or running with sanitizers), as well as some code paths that are specific to mpy-cross. I made a failed attempt to sanitize & cover the mpy-cross program in #17997, where it was suggested to make the code paths reachable from inside the coverage build.
I hope that this test will increase coverage, at least in lines. It might decrease coverage in percentage of coverable lines because there will be new coverable files and lines added.
Testing
I ran the added tests locally. CI will run them under more circumstances, as well as under UBsan and Asan.
Trade-offs and Alternatives
I initially tried to share code between
mpy-cross/main.c
and the new compiler-wrapper, but gave up on this.I observed that native code always had a lot of zeros in it, because it is rounded up to page size when mmap()'d. I re-organized the way that the size of a mmap'd area is recorded so that the actual size can still be used.
I initially printed the size of mpy-compiled files and ran the debug emitter. In the end I removed this, as it's probably fragile and (in the case of the debug emitter) extremely verbose. Consequently, the debug emitter is not actually tested at all (and maybe should be disabled)