[class] handle existing readonly properties in StoreOwnIC [v8/v8 : main]
Joyee Cheung (Gerrit)
Attention is currently required from: Shu-yu Guo, Igor Sheludko.
5 comments:
Patchset:
PTAL, this fix so far works with the existing tests and the tests I added, but I have a question regarding how to handle LookupIterator::TRANSITION when implementing [[DefineOwnProperty]] semantics in the slow path (the current patch might fail if more tests with e.g. interceptors or access checks are added), so I want to confirm the current approach is correct before diving deeper.
File src/ic/ic.cc:
Patch Set #11, Line 1762:
return JSProxy::DefineOwnProperty(it->isolate(), it->GetHolder<JSProxy>(),I *think* it should be fine to leave this for future patches?
Patch Set #11, Line 1771:
// TODO(joyee): check if Object::AddDataProperty() is enough, or if weThis part is incomplete and should be addressed in this patch - but I want to confirm that implementing an equivalent of JSObject::DefineOwnPropertyIgnoreAttributes() that plays well with the transition is the right way to go before diving into it. A lot of code used by JSObject::DefineOwnPropertyIgnoreAttributes() assumes that it->state() isn't TRANSITION, but as far as I can tell we do need to implement something that works with it for [[DefineOwnProperty]] semantics to work in the slow path.
Patch Set #11, Line 1803:
if (IsStoreOwnIC()) {This, the TODO below and the TODO in keyed-store-generic.cc all points to https://docs.google.com/document/d/1jvSEvXFHRkxg4JX-j6ho3nRqAF8vZI2Ai7RI8AY54gM/edit, I think the renaming should be done, but it can be done independently from this fix.
File test/mjsunit/regress/regress-v8-12421.js:
Is there a way to tell the test runner to run this test again with --no-lazy-feedback? (Other than copy-pasting this file and adding a // Flags: --no-lazy-feedback on top..)
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Shu-yu Guo, Igor Sheludko.
1 comment:
File test/mjsunit/regress/regress-v8-12421.js:
Patch Set #11, Line 206:
assertEquals("written", proxy.normalField);I had some tests that verify the content of trapCalls, but it got lost when I was tweaking the patch. Note to self to add them back.
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: Joyee Cheung, Igor Sheludko.
3 comments:
File src/ic/ic.cc:
Patch Set #11, Line 1762:
return JSProxy::DefineOwnProperty(it->isolate(), it->GetHolder<JSProxy>(),I *think* it should be fine to leave this for future patches?
Defer to ishell@ here. Does LookupIterator call user code or do effectful things? It seems okay by my reading.
Patch Set #11, Line 1771:
// TODO(joyee): check if Object::AddDataProperty() is enough, or if we
This part is incomplete and should be addressed in this patch - but I want to confirm that implement […]
Doesn't JSObject::DefineOwnPropertyIgnoreAttributes already handle NOT_FOUND and TRANSITION at the bottom via Object::AddDataProperty? Is this case necessary?
Patch Set #11, Line 1803:
if (IsStoreOwnIC()) {
This, the TODO below and the TODO in keyed-store-generic.cc all points to https://docs.google. […]
+1 to keeping the rename CL separate.
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
File src/ic/ic.cc:
Patch Set #11, Line 1771:
// TODO(joyee): check if Object::AddDataProperty() is enough, or if weDoesn't JSObject::DefineOwnPropertyIgnoreAttributes already handle NOT_FOUND and TRANSITION at the b […]
Currently NOT_FOUND and TRANSITION are UNREACHABLE in the switches in JSObject::DefineOwnPropertyIgnoreAttributes (before it hits Object::AddDataProperty at the bottom), so for example with --no-lazy-feedback the globalThis tests added in this CL would crash in JSObject::DefineOwnPropertyIgnoreAttributes (because it would be updated from ACCESS_CHECK to TRANSITION). I do wonder if we could just add some code there to handle these two cases directly in JSObject::DefineOwnPropertyIgnoreAttributes instead of having a new implementation though, unless it's intentional that this shouldn't be done there.
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: Joyee Cheung, Igor Sheludko.
1 comment:
File src/ic/ic.cc:
Patch Set #11, Line 1771:
// TODO(joyee): check if Object::AddDataProperty() is enough, or if we
Currently NOT_FOUND and TRANSITION are UNREACHABLE in the switches in JSObject::DefineOwnPropertyIgn […]
I see. NOT_FOUND is already handled, though, since the for loop in DefineOwnPropertyIgnoreAttribute exits on !it->IsFound().
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Shu-yu Guo, Igor Sheludko.
4 comments:
Patchset:
I tried to come up with some test cases for other states being transitioned, but reading StoreIC::LookupForWrite I realized that it seems only possible to transition from NOT_FOUND and ACCESS_CHECK here, so I added some UNREACHABLE checks and DCHECKs for the impossible cases instead. I think this is ready now, also updated the proxy case a bit to make sure that the defineProperty traps are called before an error is thrown. Pinging Igor for a review.
File src/ic/ic.cc:
Patch Set #11, Line 1771:
// TODO(joyee): check if Object::AddDataProperty() is enough, or if we
I see. […]
Ah, good point, I updated to use JSObject::DefineOwnPropertyIgnoreAttributes for NOT_FOUND too.
File test/mjsunit/regress/regress-v8-12421.js:
Is there a way to tell the test runner to run this test again with --no-lazy-feedback? (Other than c […]
I added a test that just reload this one with --no-lazy-feedback-allocation.
Patch Set #11, Line 206:
assertEquals("written", proxy.normalField);
I had some tests that verify the content of trapCalls, but it got lost when I was tweaking the patch […]
Done
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: Joyee Cheung, Igor Sheludko.
Patch set 13:Code-Review +1
1 comment:
Patchset:
lgtm but let's wait for ishell@'s review who knows the IC system much better than I do.
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Igor Sheludko.
1 comment:
Patchset:
Pinging Igor for a review, this is breaking the v9.7 release
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Shu-yu Guo (Gerrit)
Attention is currently required from: Joyee Cheung, Igor Sheludko.
1 comment:
Patchset:
Pinging Igor for a review, this is breaking the v9. […]
Hm, that's already released, right? So we'd need to prepare a backport when this lands.
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Igor Sheludko.
1 comment:
Patchset:
Hm, that's already released, right? So we'd need to prepare a backport when this lands.
Yes, we should request for a backport.
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Toon Verwaest (Gerrit)
Attention is currently required from: Shu-yu Guo, Joyee Cheung, Igor Sheludko.
6 comments:
Patchset:
Ugh, reusing StoreIC for StoreOwn and then even defineOwnProperty seems like a complete error-prone mess. Are you sure this is how we want to do it?
File src/ic/ic.cc:
Patch Set #11, Line 1762:
return JSProxy::DefineOwnProperty(it->isolate(), it->GetHolder<JSProxy>(),Defer to ishell@ here. […]
This is fine.
File src/ic/ic.cc:
Patch Set #13, Line 1748:
if (!Object::CheckContextualStoreToJSGlobalObject(it, should_throw)) {This should never be possible. A contextual store is e.g., "a = 10" that ends up targeting the native context (global object).
Patch Set #13, Line 1871:
// traps need to be called first if they are present.The same is true for interceptors?
Patch Set #13, Line 1881:
UpdateCaches(&it, value, store_origin);Won't the proxy IC end up calling the store trap instead of the define trap?
I tested in on 9.9 and that's indeed what it seems to be doing due to SetProperty below as well; but the IC will now cause a hybrid between set and defineProperty...:
d8> Proxy.prototype = Object.prototype
d8> Z = class Z extends Proxy { constructor() { super({}, {set() {
console.log("set"); return true }, defineProperty() {
console.log("defineProperty"); return true; } }) } a = 1 }
d8> new Z()
set
{}
File src/objects/js-objects.cc:
Patch Set #13, Line 199:
} else {else if
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Shu-yu Guo, Toon Verwaest, Igor Sheludko.
2 comments:
Patchset:
Ugh, reusing StoreIC for StoreOwn and then even defineOwnProperty seems like a complete error-prone […]
I agree this is error-prone, but this is what currently on the tree. So I left some TODOs in the relevant code paths saying we should either rename them (see Shu's doc https://docs.google.com/document/d/1jvSEvXFHRkxg4JX-j6ho3nRqAF8vZI2Ai7RI8AY54gM/edit), or separate them out (can probably be done following the renaming). As the doc described, what we actually need is support for Set and Define, and the name Store is a bit ambiguous. Where StoreOwn is currently used is actually hitting [[DeinfeOwnProperty]] semantics in the spec, so to get out of this weird reuse we should probably stop merging Set and Define into Store, and separate the Define part out of StoreIC. I think we can focus on fixing the behavior in this patch though, since the bug is already out there, and the renaming/separation can be a follow-up, WDYT?
File src/ic/ic.cc:
Patch Set #11, Line 1762:
return JSProxy::DefineOwnProperty(it->isolate(), it->GetHolder<JSProxy>(),
This is fine.
Done
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Shu-yu Guo, Toon Verwaest, Igor Sheludko.
4 comments:
File src/ic/ic.cc:
Patch Set #13, Line 1748:
if (!Object::CheckContextualStoreToJSGlobalObject(it, should_throw)) {
This should never be possible. A contextual store is e.g. […]
I've updated this to a DCHECK instead.
Patch Set #13, Line 1871:
// traps need to be called first if they are present.The same is true for interceptors?
When fixing this I noticed that DefineOwnPropertyIgnoreAttributes actually calls the setter interceptor, not the definer. Is that intentional? Should we update DefineOwnPropertyIgnoreAttributes to call the definer interceptor to fix this (I wonder if that's a breaking change and should be left to another patch), or just add some special handling for the StoreOwnIC to call the definer in this patch?
Patch Set #13, Line 1881:
UpdateCaches(&it, value, store_origin);
Won't the proxy IC end up calling the store trap instead of the define trap? […]
With this patch this test should go into DefineOwnDataProperty below, so only the defineProperty would be called (previously it went into Object::SetProperty which was part of the bug and triggered the set trap). I added a test for this case.
File src/objects/js-objects.cc:
Patch Set #13, Line 199:
} else {else if
Done
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Shu-yu Guo, Toon Verwaest, Igor Sheludko.
Patch set 16:-Commit-Queue
1 comment:
Patchset:
I agree this is error-prone, but this is what currently on the tree. […]
Accidentally clicked the resolved mark, mark it as unresolved.
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Toon Verwaest (Gerrit)
Attention is currently required from: Shu-yu Guo, Joyee Cheung, Igor Sheludko.
3 comments:
Patchset:
Accidentally clicked the resolved mark, mark it as unresolved.
Basically StoreIC was just there for JS set semantics. StoreOwn was a hack to reuse the StoreICs _only_ for the case where the target is an engine-controlled object literal, in which case we know that DefineOwn and StoreOwn have the exact same semantics (and no proxy traps can be triggered etc). Extending this to mean Define for user-controlled objects (field defines on any random JSReceiver) seems like a minor change but really isn't; as is clear from all the resulting security and semantical bugs. Beyond simply renaming, I think we should really pull those bits apart instead.
File src/ic/ic.cc:
Patch Set #13, Line 1871:
// traps need to be called first if they are present.When fixing this I noticed that DefineOwnPropertyIgnoreAttributes actually calls the setter intercep […]
When this was implemented the method was still called SetOwnPropertyIgnoreAttributes and there was no definer interceptor yet. Changing it over sounds ok to me, but might break some embedder?
Patch Set #13, Line 1881:
UpdateCaches(&it, value, store_origin);
With this patch this test should go into DefineOwnDataProperty below, so only the defineProperty wou […]
I mean UpdateCaches will for the Proxy case install the Proxy StoreIC state, which will in turn (once warmed up) call the "set" trap.
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Toon Verwaest (Gerrit)
Attention is currently required from: Shu-yu Guo, Joyee Cheung, Igor Sheludko.
1 comment:
Patchset:
marking unresolved
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Toon Verwaest (Gerrit)
Attention is currently required from: Shu-yu Guo, Joyee Cheung, Igor Sheludko.
1 comment:
File src/ic/ic.cc:
Patch Set #13, Line 1881:
UpdateCaches(&it, value, store_origin);I mean UpdateCaches will for the Proxy case install the Proxy StoreIC state, which will in turn (onc […]
unresolved
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Shu-yu Guo, Toon Verwaest, Igor Sheludko.
1 comment:
File src/ic/ic.cc:
Patch Set #13, Line 1881:
UpdateCaches(&it, value, store_origin);
unresolved
I see, I tested it with --no-lazy-feedback-allocation and indeed it triggers the set trap. I tried to fix that by installing the slow handler instead of the kProxy SMI handler for StoreOwnIC (we already install the slow handler for the private DefineOwnIC, so it seems logical to me), and fixing the slow stub to use CreateDataProperty instead of DefineOwnPropertyIgnoreAttributes, and it fixes the test.
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Shu-yu Guo, Toon Verwaest, Igor Sheludko.
1 comment:
Patchset:
Beyond simply renaming, I think we should really pull those bits apart instead.
I agree. But should that be done here though? My and Shu's opinion (https://chromium-review.googlesource.com/c/v8/v8/+/3300092/comment/18d37d0d_a6f0b8c1/) is that we can leave the renaming as a follow-up, since this messy reuse is already in the tree, and therefor I think a real refactoring should be done separately from this patch, too. Perhaps I should open a separate issue to track this refactoring?
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Toon Verwaest (Gerrit)
Attention is currently required from: Shu-yu Guo, Joyee Cheung, Igor Sheludko.
2 comments:
Patchset:
> Beyond simply renaming, I think we should really pull those bits apart instead. […]
That sounds alright. Yeah I'm not suggesting to do it here; I'm just a bit unhappy that we got to this point / design.
File src/ic/ic.cc:
Patch Set #17, Line 1982:
return MaybeObjectHandle(StoreHandler::StoreInterceptor(isolate()));Given that we have a definer, as you mentioned, we should also call the definer here in case of StoreOwn?
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Shu-yu Guo, Toon Verwaest, Igor Sheludko.
6 comments:
Patchset:
That sounds alright. […]
Yeah in hindsight we probably should've put this behind a flag first..I'll try to follow up with a refactoring to sort it out, thanks for the bearing with my questions!
Patchset:
I think I've fixed the interceptor case and the proxy case, PTAL, thanks!
File src/ic/ic.cc:
Patch Set #11, Line 1803:
if (IsStoreOwnIC()) {
+1 to keeping the rename CL separate.
Done
File src/ic/ic.cc:
Patch Set #13, Line 1871:
// traps need to be called first if they are present.When this was implemented the method was still called SetOwnPropertyIgnoreAttributes and there was n […]
I checked it and it seems simply doing Object.defineProperty would result in an invocation of the setter interceptor, I suspect that changing this behavior entails a deprecation, so I left a TODO to fix DefineOwnPropertyIgnoreAttributes later instead. Also I realized that objects with interceptors cannot prevent extension, so I only added a test for non-configurable objects.
Patch Set #13, Line 1881:
UpdateCaches(&it, value, store_origin);
I see, I tested it with --no-lazy-feedback-allocation and indeed it triggers the set trap. […]
Done
File src/ic/ic.cc:
Patch Set #17, Line 1982:
return MaybeObjectHandle(StoreHandler::StoreInterceptor(isolate()));
Given that we have a definer, as you mentioned, we should also call the definer here in case of Stor […]
I used the slow-case Smi handler here for StoreOwnIC, which would invoke the definer accordingly.
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Toon Verwaest (Gerrit)
Attention is currently required from: Shu-yu Guo, Joyee Cheung, Igor Sheludko.
3 comments:
File src/ic/ic.cc:
Patch Set #19, Line 1806:
switch (it->state()) {What's the reason to duplicate it here rather than to update DefineOwnPropertyIgnoreAttributes? Given that name I'd expect it should always have define semantics and hence call the definer. At least we could pass a flag into that method to indicate whether define or set semantics should be used instead of duplicating this here.
Patch Set #19, Line 1814:
if (it->HolderIsReceiverOrHiddenPrototype()) {We're in StoreOwn; this should be a guarantee? The LookupIterator is configured to only look at LookupIterator::OWN.
Patch Set #19, Line 1926:
.IsUndefined(it.isolate()))) {CheckIfCanDefine with also call the query interceptor if present. Can we just skip entirely if we have an interceptor?
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Shu-yu Guo, Toon Verwaest, Igor Sheludko.
Patch set 20:Commit-Queue +1
3 comments:
File src/ic/ic.cc:
Patch Set #19, Line 1806:
switch (it->state()) {
What's the reason to duplicate it here rather than to update DefineOwnPropertyIgnoreAttributes? Give […]
Was just trying to not break DefineOwnPropertyIgnoreAttributes
if possible, but adding a flag is probably a better idea - I've added a EnforceDefineSemantics flag for the special cases.
Patch Set #19, Line 1814:
if (it->HolderIsReceiverOrHiddenPrototype()) {
We're in StoreOwn; this should be a guarantee? The LookupIterator is configured to only look at Look […]
Done
Patch Set #19, Line 1926:
.IsUndefined(it.isolate()))) {
CheckIfCanDefine with also call the query interceptor if present. […]
Done
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Toon Verwaest (Gerrit)
Attention is currently required from: Shu-yu Guo, Joyee Cheung, Igor Sheludko.
Patch set 20:Code-Review +1
2 comments:
Patchset:
Thanks Joyee! lgtm from my side
File src/objects/js-objects.h:
Patch Set #20, Line 416:
enum class EnforceDefineSemantics { kLegacy, kDefine };I'd say just call this kSet for now; since it's not clear we'll actually move this over.
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
Joyee Cheung (Gerrit)
Attention is currently required from: Shu-yu Guo, Igor Sheludko.
Patch set 21:Commit-Queue +2
3 comments:
Patchset:
Yeah in hindsight we probably should've put this behind a flag first.. […]
Opened https://bugs.chromium.org/p/v8/issues/detail?id=12548 to follow up.
Patchset:
Thanks for the reviews!
File src/objects/js-objects.h:
Patch Set #20, Line 416:
enum class EnforceDefineSemantics { kLegacy, kDefine };I'd say just call this kSet for now; since it's not clear we'll actually move this over.
Done
To view, visit change 3300092. To unsubscribe, or for help writing mail filters, visit settings.
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted this change.
20 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/objects/js-objects.cc
Insertions: 1, Deletions: 1.
@@ -3436,7 +3436,7 @@
result = DefinePropertyWithInterceptorInternal(
it, it->GetInterceptor(), should_throw, &descriptor);
} else {
- DCHECK_EQ(semantics, EnforceDefineSemantics::kLegacy);
+ DCHECK_EQ(semantics, EnforceDefineSemantics::kSet);
if (handling == DONT_FORCE_FIELD) {
result =
JSObject::SetPropertyWithInterceptor(it, should_throw, value);
```
```
The name of the file: src/objects/js-objects.h
Insertions: 3, Deletions: 3.
@@ -413,19 +413,19 @@
// kDefine allows us to implement the define semantics correctly
// in selected locations.
// TODO(joyee): see if we can deprecate the old behavior.
- enum class EnforceDefineSemantics { kLegacy, kDefine };
+ enum class EnforceDefineSemantics { kSet, kDefine };
V8_WARN_UNUSED_RESULT static MaybeHandle<Object>
DefineOwnPropertyIgnoreAttributes(
LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
AccessorInfoHandling handling = DONT_FORCE_FIELD,
- EnforceDefineSemantics semantics = EnforceDefineSemantics::kLegacy);
+ EnforceDefineSemantics semantics = EnforceDefineSemantics::kSet);
V8_WARN_UNUSED_RESULT static Maybe<bool> DefineOwnPropertyIgnoreAttributes(
LookupIterator* it, Handle<Object> value, PropertyAttributes attributes,
Maybe<ShouldThrow> should_throw,
AccessorInfoHandling handling = DONT_FORCE_FIELD,
- EnforceDefineSemantics semantics = EnforceDefineSemantics::kLegacy);
+ EnforceDefineSemantics semantics = EnforceDefineSemantics::kSet);
V8_WARN_UNUSED_RESULT static MaybeHandle<Object> V8_EXPORT_PRIVATE
SetOwnPropertyIgnoreAttributes(Handle<JSObject> object, Handle<Name> name,
```
[class] handle existing readonly properties in StoreOwnIC
Previously, StoreOwnIC incorrectly reuses the [[Set]] semantics
when initializing public literal class fields and object literals in
certain cases (e.g. when there's no feedback).
This was less of an issue for object literals, but with public class
fields it's possible to define property attributes while the
instance is still being initialized, or to encounter existing static
"name" or "length" properties that should be readonly. This patch
fixes it by
1) Emitting code that calls into the slow stub when
handling StoreOwnIC with existing read-only properties.
2) Adding extra steps in StoreIC::Store to handle such stores
properly with [[DefineOwnProperty]] semantics.
Bug: v8:12421, v8:9888
Change-Id: I6547320a1caba58c66ee1043cd3183a2de7cefef
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3300092
Reviewed-by: Shu-yu Guo <s...@chromium.org>
Reviewed-by: Toon Verwaest <verw...@chromium.org>
Commit-Queue: Joyee Cheung <jo...@igalia.com>
Cr-Commit-Position: refs/heads/main@{#78659}
---
M src/objects/js-objects.cc
M src/ic/ic.cc
M src/objects/js-objects.h
A test/mjsunit/regress/regress-v8-12421.js
M src/objects/objects.h
M src/objects/lookup.cc
A test/mjsunit/regress/regress-v8-12421-no-lazy-feedback.js
M src/ic/keyed-store-generic.cc
A test/mjsunit/proxy-super-return-define-property-trap.js
M src/objects/objects.cc
M test/cctest/test-api-interceptors.cc
11 files changed, 755 insertions(+), 56 deletions(-)