Conversation
|
Thanks for your pull request, @MartinNowak! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. |
MartinNowak
force-pushed
the
cdcmp_test
branch
from
b6b8e7c to
455ef1a
Compare
| /* mov rbp,rsp */ 0x48, 0x8b, 0xec, | ||
| /* xor eax,eax */ 0x31, 0xc0, | ||
| /* pop rbp */ 0x5d, | ||
| /* ret */ 0xc3, |
There was a problem hiding this comment.
These kinds of tests can be extremely tedious to maintain, which is why I abandoned doing them in the past.
They can be much easier if, instead, the object file is run through obj2asm, and the result diffed against a known good file.
There was a problem hiding this comment.
FYI: the file ships with a generator - it's automatically generate and whenever the codegen changes can be easily, but consciously upgraded.
There was a problem hiding this comment.
Did you see this comment?
Run
env DMD=generated/linux/release/64/dmd rdmd -version=update test/runnable/test_cdcmp.dafter codegen changes.
There was a problem hiding this comment.
No, I didn't notice it was auto-generated. Thanks for letting me know.
| Code!(ubyte, "<", ubyte)([ | ||
| /* push rbp */ 0x55, | ||
| /* mov rbp,rsp */ 0x48, 0x8b, 0xec, | ||
| /* sub rsp,0x10 */ 0x48, 0x83, 0xec, 0x10, |
There was a problem hiding this comment.
All the 0xNN data can get incredibly tedious to create. In the future, use hexStrings -
0x48, 0x83, 0xEC, 0x10,
becomes:
hexString!"48 83 EC 10"
There was a problem hiding this comment.
This file is auto-generated ;-)
There was a problem hiding this comment.
HexString relies on phobos, so I avoided that.
| import std.regex : ctRegex, matchFirst, replaceFirstInto; | ||
| import std.stdio : File, stdout, writeln; | ||
| import std.string : strip; | ||
| import std.typecons : tuple, EnumMembers; |
There was a problem hiding this comment.
This means in order for this test file to work, pretty much all of Phobos has to work first. But the dmd tests need to pass before getting Phobos to work, because it is much much easier to debug a code gen problem in test/runnable than it is to try to isolate problems down in Phobos.
There was a problem hiding this comment.
A better approach is to do it like test/runnable/iasm.d does, where a pointer to the start of the function is taken and then the generated bytes are compared.
There was a problem hiding this comment.
No. It's just to automatically update the code blocks. Note version(update) block.
We have a few of these files already in the testsuite ;-)
There was a problem hiding this comment.
A better approach is to do it like test/runnable/iasm.d does, where a pointer to the start of the function is taken and then the generated bytes are compared.
That's how it's done for the testees, it was just to tedious to update the expectated outcome, so I added an automated update part using objdump. This allows easy diffing and turned out very helpful for the cdvecfill changes where you can see a commit-by-commit diff (https://github.com/dlang/dmd/pull/7003/commits).
wilzbach
left a comment
wilzbach
left a comment
There was a problem hiding this comment.
Ready whenever you are.
MartinNowak
force-pushed
the
cdcmp_test
branch
from
455ef1a to
aab2201
Compare
|
We should look to create some small tools/scripts to reduce update boilerplate for those tests, maybe incorporating it into d_do_test or make it a sidekick. |
4 participants
will need a rebase after #7848