Use correct field offset in genPutArgStkFieldList#18483
Conversation
| @@ -0,0 +1,37 @@ | |||
| using System; | |||
| getEmitter()->emitIns_S_R(ins_Store(type), attr, reg, outArgVarNum, argOffset); | ||
| argOffset += EA_SIZE_IN_BYTES(attr); | ||
| // argument area. | ||
| unsigned thisArgOffset = argOffset + fieldListPtr->gtFieldOffset; |
There was a problem hiding this comment.
I'm not exactly familiar with this code but I'm pretty sure that here "this" does not refer to the this argument. Perhaps argFieldOffset would be a better name? argOffset is the offset of the an argument composed from multiple fields and argFieldOffset is the offset of an individual field.
There was a problem hiding this comment.
Did this fix get pushed? Github says the local is still named thisFieldOffset?
I like argFieldOffset here better too.
|
@dotnet/jit-contrib PTAL |
|
@dotnet-bot test Ubuntu x64 Checked corefx_baseline |
|
I'm really puzzled why this wasn't caught in our normal CoreCLR CI.... |
Me too - I would think that this would be a pretty fundamental kind of test to have around. |
|
Maybe you could try an altjit pmi run (using I haven't tried altjit pmi yet so you might hit some snags. If so, I'd be happy to jump in and fix them. |
|
@dotnet-bot test Ubuntu x64 Checked corefx_baseline |
|
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test OSX machine broke the connection. |
|
Seems like this test case should have been covered with tests/src/JIT/Directed/StructABI/StructABI.cs. @CarolEidt, if we are adding a new test it is worth adding it to the Arm64 and Arm32 lst file. This can be done in a later pr. |
jashook
left a comment
jashook
left a comment
There was a problem hiding this comment.
Minus the comment on the lst file, lgtm.
It's not run on linux: https://github.com/dotnet/coreclr/blob/master/tests/testsFailingOutsideWindows.txt#L4 |
|
That is a problem, that test has previously been passing on unix. It should be re-enabled asap, it is one of our best tests for these ABI issues. |
OK, I think I'm going to let the rest of the tests finish, especially the corefx tests, since they've finally begun (CI seems a bit slow this morning) |
|
Wow looks like we have not been running the abi test for a while, that is extremely surprising. That is very unfortunate. |
|
It seems that it was "temporarily" disabled over two and a half years ago here: 4ddcf19#diff-189e5cf450f28bd675ab3c19f246c9a6 But it doesn't seem that any issue was filed to track re-enabling it. |
|
Looking into this now. I will try to re-enable the test, hopefully it has not regressed. |
|
I filed #18485 |
|
As far as I can tell there was a process error in https://ci.dot.net/job/dotnet_coreclr/job/master/job/jitstress/job/x64_checked_ubuntu_corefx_baseline_flow_prtest/31/ |
|
@dotnet-bot test Ubuntu x64 Checked corefx_baseline |
|
@jashook could you take a look at the .lst file changes to make sure I've got it right? |
|
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test |
|
@dotnet/dnceng - The corefx test failure seems to be process-related. Is this a known problem? |
|
The Windows_NT x64 Checked Innerloop Build and Test is presumably https://github.com/dotnet/coreclr/issues/18467, but will retry. |
|
@dotnet-bot test Windows_NT x64 Checked Innerloop Build and Test |
|
@CarolEidt I believe @chcosta was looking at this, and from the very low build number the conclusion was that this job had never actually worked. Chris can add context if he found any (should probably disable this leg) |
|
@dotnet/jit-contrib - is there a different job I should request for corefx tests? Or are this simply not working? |
|
It's simply not working; I would just ignore it for now, as it's not providing you any "Should I check in?" value, and there are issues tracking fixing it. |
|
Looks like there's a netci.groovy configuration problem. (I probably introduced it with some of my refactoring work.) I opened an issue with details here: https://github.com/dotnet/coreclr/issues/18498. |
* Use correct field offset in genPutArgStkFieldList Fix dotnet/coreclr#18482 * formatting * Add the new test to the arm and arm64 test lists Commit migrated from dotnet/coreclr@13954f2
7 participants
Fix #18482