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

write-barrier slow path sharing CLs did not block LR in all necessary places on ARM #34411

Closed
mraleph opened this issue Sep 9, 2018 · 3 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P0 A serious issue requiring immediate resolution

Comments

@mraleph
Copy link
Member

mraleph commented Sep 9, 2018

On ARM we

I see that StoreInstanceFieldInstr takes care to reserve it, but StoreStaticFieldInstr and StoreIndexedInstr does not seem to do the same.

Also I noticed that StoreInstanceFieldInstr has the following problem: even if ShouldEmitStoreBarrier() is false we might still emit a store barrier - because this might be an unboxed store (e.g. obj.field = 2.0) in which case we don't need a store barrier for the value, but we need a store barrier for the temporary box - this means that LR needs to be blocked for these cases as well.

I think StoreIntoObject on ARM needs to assert that object != LR - to make sure that we catch any remaining cases where this does not hold.

[I am marking this as a P0 because this might lead to hard to debug issues where LR is overwritten with strange values]

@mraleph mraleph added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P0 A serious issue requiring immediate resolution labels Sep 9, 2018
@mraleph mraleph changed the title write-barrier sharing CLs did not block LR in all necessary places on ARM write-barrier slow path sharing CLs did not block LR in all necessary places on ARM Sep 9, 2018
@rmacnak-google
Copy link
Contributor

Maybe we should we just make LR unavailable to register allocation on ARM like we do on ARM64. It seems easy for all sorts of slow path code to incorrectly assume it's available for making calls.

@mraleph
Copy link
Member Author

mraleph commented Sep 11, 2018

@rmacnak-google True, this indeed would be an even more robust approach. I think one of the original suggestions was to make TMP = LR to avoid blocking yet another register.

@sjindel-google
Copy link
Contributor

Many places in the ARM assembler and compiler use IP directly rather than TMP.

dart-bot pushed a commit that referenced this issue Sep 11, 2018
Skip barrier for x.slot = x. Note that box allocations are non-Smi.

Bug: #34411
Change-Id: I846639747f0b583c24cb6ecf40548cf32b76d580
Reviewed-on: https://dart-review.googlesource.com/74324
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Samir Jindel <sjindel@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
tekknolagi pushed a commit to tekknolagi/dart-assembler that referenced this issue Nov 3, 2020
Skip barrier for x.slot = x. Note that box allocations are non-Smi.

Bug: dart-lang#34411
Change-Id: I846639747f0b583c24cb6ecf40548cf32b76d580
Reviewed-on: https://dart-review.googlesource.com/74324
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Samir Jindel <sjindel@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
tekknolagi pushed a commit to tekknolagi/dart-assembler that referenced this issue Nov 3, 2020
Flutter Gallery release:
Instructions(CodeSize): 5668368 -> 5670880 (+0.044%)

Fixes dart-lang#34411

Bug: dart-lang#34411
Change-Id: I5281ac9609ec5953cdaa881226f7034a5697d0ce
Reviewed-on: https://dart-review.googlesource.com/74923
Commit-Queue: Ryan Macnak <rmacnak@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P0 A serious issue requiring immediate resolution
Projects
None yet
Development

No branches or pull requests

3 participants