build: Add seam to enable array bounds checking [chromium/src : main]
Kalvin Lee (Gerrit)
Kalvin Lee added 1 comment![]( "Open in Gerrit")
Hi Nico - I stole this arcana from Daniel, who claims he got it from you. Would you say this is leaning in the right direction?
Related details
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
- Kalvin Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kalvin Lee (Gerrit)
Kalvin Lee added 1 comment![]( "Open in Gerrit")
Which embedders want this? Why?
[ANGLE asked for this](https://groups.google.com/g/angleproject/c/b3waFJdP4Wc/m/xyfKFfx7AgAJ).
Related details
- Nico Weber
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
- Kalvin Lee
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shahbaz Youssefi (Gerrit)
Shahbaz Youssefi added 1 comment![]( "Open in Gerrit")
syouseffi: Why do you need this for angle?
ANGLE is perf sensitive. Outside Chromium, we don't want to have the overhead.
Related details
- Kalvin Lee
- Nico Weber
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
Shahbaz Youssefisyouseffi: Why do you need this for angle?
ANGLE is perf sensitive. Outside Chromium, we don't want to have the overhead.
Did you measure an overhead?
Chromium is also perf-sensitive, and this looks neutral in benchmarks. See linked docs.
Related details
- Kalvin Lee
- Shahbaz Youssefi
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shahbaz Youssefi (Gerrit)
Shahbaz Youssefi added 1 comment![]( "Open in Gerrit")
Shahbaz Youssefisyouseffi: Why do you need this for angle?
Nico WeberANGLE is perf sensitive. Outside Chromium, we don't want to have the overhead.
Did you measure an overhead?
Chromium is also perf-sensitive, and this looks neutral in benchmarks. See linked docs.
I could have someone gather some data if you have instructions. But FYI, we are _really_ perf sensitive as the GLES driver on Android, and under high scrutiny. To the point where likely/unlikely markers are needed, we need to hand-hold the compiler to inline somethings and somethings not, etc.
We don't need bounds checking as a GLES driver, and I'm fairly confident someone will come back to us and ask for it to be removed.
Related details
- Kalvin Lee
- Nico Weber
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
Shahbaz Youssefisyouseffi: Why do you need this for angle?
Nico WeberANGLE is perf sensitive. Outside Chromium, we don't want to have the overhead.
Shahbaz YoussefiDid you measure an overhead?
Chromium is also perf-sensitive, and this looks neutral in benchmarks. See linked docs.
I could have someone gather some data if you have instructions. But FYI, we are _really_ perf sensitive as the GLES driver on Android, and under high scrutiny. To the point where likely/unlikely markers are needed, we need to hand-hold the compiler to inline somethings and somethings not, etc.
We don't need bounds checking as a GLES driver, and I'm fairly confident someone will come back to us and ask for it to be removed.
To the point where likely/unlikely markers are needed
A bit of a derail, but do you build with PGO? That helps quite a bit with perf, and should make those markers unnecessary.
Related details
- Kalvin Lee
- Shahbaz Youssefi
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shahbaz Youssefi (Gerrit)
Shahbaz Youssefi added 1 comment![]( "Open in Gerrit")
Shahbaz Youssefisyouseffi: Why do you need this for angle?
Nico WeberANGLE is perf sensitive. Outside Chromium, we don't want to have the overhead.
Shahbaz YoussefiDid you measure an overhead?
Chromium is also perf-sensitive, and this looks neutral in benchmarks. See linked docs.
Nico WeberI could have someone gather some data if you have instructions. But FYI, we are _really_ perf sensitive as the GLES driver on Android, and under high scrutiny. To the point where likely/unlikely markers are needed, we need to hand-hold the compiler to inline somethings and somethings not, etc.
We don't need bounds checking as a GLES driver, and I'm fairly confident someone will come back to us and ask for it to be removed.
To the point where likely/unlikely markers are needed
A bit of a derail, but do you build with PGO? That helps quite a bit with perf, and should make those markers unnecessary.
We considered it, that all actually started with the idea of PGO. But we found PGO to be cumbersome; it has to be constantly updated (which we don't have the resources for), and apparently can be detrimental when it's out of date. We had an excellent engineer who was figuring out what PGO is doing it, and manually doing it in the code so it's baked in and we don't have to rely on PGO.
Well, I see "we", I mean the ANGLE-4-Android folks.
Related details
- Kalvin Lee
- Nico Weber
- Yuly Novikov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 2 comments![]( "Open in Gerrit")
Hm, looks like the
Shahbaz Youssefisyouseffi: Why do you need this for angle?
Nico WeberANGLE is perf sensitive. Outside Chromium, we don't want to have the overhead.
Shahbaz YoussefiDid you measure an overhead?
Chromium is also perf-sensitive, and this looks neutral in benchmarks. See linked docs.
Nico WeberI could have someone gather some data if you have instructions. But FYI, we are _really_ perf sensitive as the GLES driver on Android, and under high scrutiny. To the point where likely/unlikely markers are needed, we need to hand-hold the compiler to inline somethings and somethings not, etc.
We don't need bounds checking as a GLES driver, and I'm fairly confident someone will come back to us and ask for it to be removed.
Shahbaz YoussefiTo the point where likely/unlikely markers are needed
A bit of a derail, but do you build with PGO? That helps quite a bit with perf, and should make those markers unnecessary.
We considered it, that all actually started with the idea of PGO. But we found PGO to be cumbersome; it has to be constantly updated (which we don't have the resources for), and apparently can be detrimental when it's out of date. We had an excellent engineer who was figuring out what PGO is doing it, and manually doing it in the code so it's baked in and we don't have to rely on PGO.
Well, I see "we", I mean the ANGLE-4-Android folks.
Saying "we care about perf so much that we need an opt-out for a thing that has no measurable cost" and "we don't use PGO" is IMHO not a very convincing position.
I'd say we land the flag without an opt-out for now, and add it if there's data that shows that we actually need it.
Related details
- Kalvin Lee
- Shahbaz Youssefi
- Yuly Novikov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
Hm, looks like the
(accidental comment, ignore this one)
Yuly Novikov (Gerrit)
Yuly Novikov added 1 comment![]( "Open in Gerrit")
Would it be acceptable if we remove the "//build/config/compiler:sanitize_c_array_bounds" config in standalone ANGLE, like we do for others here https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/gni/angle.gni;l=352;drc=3c2bff4b540d1c98b6dd4ea901b05a3b7e6112b8?
Related details
- Kalvin Lee
- Shahbaz Youssefi
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shahbaz Youssefi (Gerrit)
Shahbaz Youssefi added 2 comments![]( "Open in Gerrit")
Shahbaz Youssefisyouseffi: Why do you need this for angle?
Nico WeberANGLE is perf sensitive. Outside Chromium, we don't want to have the overhead.
Shahbaz YoussefiDid you measure an overhead?
Chromium is also perf-sensitive, and this looks neutral in benchmarks. See linked docs.
Nico WeberI could have someone gather some data if you have instructions. But FYI, we are _really_ perf sensitive as the GLES driver on Android, and under high scrutiny. To the point where likely/unlikely markers are needed, we need to hand-hold the compiler to inline somethings and somethings not, etc.
We don't need bounds checking as a GLES driver, and I'm fairly confident someone will come back to us and ask for it to be removed.
Shahbaz YoussefiTo the point where likely/unlikely markers are needed
A bit of a derail, but do you build with PGO? That helps quite a bit with perf, and should make those markers unnecessary.
Nico WeberWe considered it, that all actually started with the idea of PGO. But we found PGO to be cumbersome; it has to be constantly updated (which we don't have the resources for), and apparently can be detrimental when it's out of date. We had an excellent engineer who was figuring out what PGO is doing it, and manually doing it in the code so it's baked in and we don't have to rely on PGO.
Well, I see "we", I mean the ANGLE-4-Android folks.
Saying "we care about perf so much that we need an opt-out for a thing that has no measurable cost" and "we don't use PGO" is IMHO not a very convincing position.
I'd say we land the flag without an opt-out for now, and add it if there's data that shows that we actually need it.
I never agreed it doesn't have a measurable cost :)
Would it be acceptable if we remove the "//build/config/compiler:sanitize_c_array_bounds" config in standalone ANGLE, like we do for others here https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/gni/angle.gni;l=352;drc=3c2bff4b540d1c98b6dd4ea901b05a3b7e6112b8?
- Kalvin Lee
- Nico Weber
- Yuly Novikov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuly Novikov (Gerrit)
Yuly Novikov added 1 comment![]( "Open in Gerrit")
Shahbaz YoussefiWould it be acceptable if we remove the "//build/config/compiler:sanitize_c_array_bounds" config in standalone ANGLE, like we do for others here https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/gni/angle.gni;l=352;drc=3c2bff4b540d1c98b6dd4ea901b05a3b7e6112b8?
Yes sure, that would be great!
OK, so the plan is:
1. Abandon this
2. Land https://chromium-review.googlesource.com/c/chromium/src/+/6600050
3. Roll Chromium into ANGLE
4. Disable "//build/config/compiler:sanitize_c_array_bounds" config in ANGLE.
Ideally we disable the config in the roll, but I think it's not a big deal if we do it a bit later, since rolling and landing would be hard to synchronize.
Related details
- Kalvin Lee
- Nico Weber
- Shahbaz Youssefi
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 2 comments![]( "Open in Gerrit")
Shahbaz Youssefisyouseffi: Why do you need this for angle?
Nico WeberANGLE is perf sensitive. Outside Chromium, we don't want to have the overhead.
Shahbaz YoussefiDid you measure an overhead?
Chromium is also perf-sensitive, and this looks neutral in benchmarks. See linked docs.
Nico WeberI could have someone gather some data if you have instructions. But FYI, we are _really_ perf sensitive as the GLES driver on Android, and under high scrutiny. To the point where likely/unlikely markers are needed, we need to hand-hold the compiler to inline somethings and somethings not, etc.
We don't need bounds checking as a GLES driver, and I'm fairly confident someone will come back to us and ask for it to be removed.
Shahbaz YoussefiTo the point where likely/unlikely markers are needed
A bit of a derail, but do you build with PGO? That helps quite a bit with perf, and should make those markers unnecessary.
Nico WeberWe considered it, that all actually started with the idea of PGO. But we found PGO to be cumbersome; it has to be constantly updated (which we don't have the resources for), and apparently can be detrimental when it's out of date. We had an excellent engineer who was figuring out what PGO is doing it, and manually doing it in the code so it's baked in and we don't have to rely on PGO.
Well, I see "we", I mean the ANGLE-4-Android folks.
Shahbaz YoussefiSaying "we care about perf so much that we need an opt-out for a thing that has no measurable cost" and "we don't use PGO" is IMHO not a very convincing position.
I'd say we land the flag without an opt-out for now, and add it if there's data that shows that we actually need it.
I never agreed it doesn't have a measurable cost :)
You haven't shown data that it does though, while Kalvin has data showing that it doesn't.
Shahbaz YoussefiWould it be acceptable if we remove the "//build/config/compiler:sanitize_c_array_bounds" config in standalone ANGLE, like we do for others here https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/gni/angle.gni;l=352;drc=3c2bff4b540d1c98b6dd4ea901b05a3b7e6112b8?
Yuly NovikovYes sure, that would be great!
OK, so the plan is:
1. Abandon this
2. Land https://chromium-review.googlesource.com/c/chromium/src/+/6600050
3. Roll Chromium into ANGLE
4. Disable "//build/config/compiler:sanitize_c_array_bounds" config in ANGLE.
Ideally we disable the config in the roll, but I think it's not a big deal if we do it a bit later, since rolling and landing would be hard to synchronize.
Please only do 4 if you have numbers to back up it having an effect (in particular, with PGO enabled). We likely do want this on in angle as it ships in chromium.
Related details
- Kalvin Lee
- Shahbaz Youssefi
- Yuly Novikov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuly Novikov (Gerrit)
Yuly Novikov added 1 comment![]( "Open in Gerrit")
Shahbaz YoussefiWould it be acceptable if we remove the "//build/config/compiler:sanitize_c_array_bounds" config in standalone ANGLE, like we do for others here https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/gni/angle.gni;l=352;drc=3c2bff4b540d1c98b6dd4ea901b05a3b7e6112b8?
Yuly NovikovYes sure, that would be great!
Nico WeberOK, so the plan is:
1. Abandon this
2. Land https://chromium-review.googlesource.com/c/chromium/src/+/6600050
3. Roll Chromium into ANGLE
4. Disable "//build/config/compiler:sanitize_c_array_bounds" config in ANGLE.
Ideally we disable the config in the roll, but I think it's not a big deal if we do it a bit later, since rolling and landing would be hard to synchronize.
Please only do 4 if you have numbers to back up it having an effect (in particular, with PGO enabled). We likely do want this on in angle as it ships in chromium.
It would be enabled in the version of ANGLE that's built as a part of Chromium, and would be disabled in standalone ANGLE, which is shipped elsewhere.
Win-Win!
Related details
- Kalvin Lee
- Nico Weber
- Shahbaz Youssefi
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nico Weber (Gerrit)
Nico Weber added 1 comment![]( "Open in Gerrit")
Shahbaz YoussefiWould it be acceptable if we remove the "//build/config/compiler:sanitize_c_array_bounds" config in standalone ANGLE, like we do for others here https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/gni/angle.gni;l=352;drc=3c2bff4b540d1c98b6dd4ea901b05a3b7e6112b8?
Yuly NovikovYes sure, that would be great!
Nico WeberOK, so the plan is:
1. Abandon this
2. Land https://chromium-review.googlesource.com/c/chromium/src/+/6600050
3. Roll Chromium into ANGLE
4. Disable "//build/config/compiler:sanitize_c_array_bounds" config in ANGLE.
Ideally we disable the config in the roll, but I think it's not a big deal if we do it a bit later, since rolling and landing would be hard to synchronize.
Yuly NovikovPlease only do 4 if you have numbers to back up it having an effect (in particular, with PGO enabled). We likely do want this on in angle as it ships in chromium.
It would be enabled in the version of ANGLE that's built as a part of Chromium, and would be disabled in standalone ANGLE, which is shipped elsewhere.
Win-Win!
Sure, works for me from a chrome PoV.
Who are contacts on the android gles driver team who can explicitly confirm that they're not interested in security hardening measures without demonstrated performance overhead? That's fairly different from what I've heard from my contacts in android land. I'd like to check with both the gles driver team and my contacts and make sure they're aligned on this.
Related details
- Kalvin Lee
- Shahbaz Youssefi
- Yuly Novikov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Shahbaz Youssefi (Gerrit)
Shahbaz Youssefi added 2 comments![]( "Open in Gerrit")
Shahbaz Youssefisyouseffi: Why do you need this for angle?
Nico WeberANGLE is perf sensitive. Outside Chromium, we don't want to have the overhead.
Shahbaz YoussefiDid you measure an overhead?
Chromium is also perf-sensitive, and this looks neutral in benchmarks. See linked docs.
Nico WeberI could have someone gather some data if you have instructions. But FYI, we are _really_ perf sensitive as the GLES driver on Android, and under high scrutiny. To the point where likely/unlikely markers are needed, we need to hand-hold the compiler to inline somethings and somethings not, etc.
We don't need bounds checking as a GLES driver, and I'm fairly confident someone will come back to us and ask for it to be removed.
Shahbaz YoussefiTo the point where likely/unlikely markers are needed
A bit of a derail, but do you build with PGO? That helps quite a bit with perf, and should make those markers unnecessary.
Nico WeberWe considered it, that all actually started with the idea of PGO. But we found PGO to be cumbersome; it has to be constantly updated (which we don't have the resources for), and apparently can be detrimental when it's out of date. We had an excellent engineer who was figuring out what PGO is doing it, and manually doing it in the code so it's baked in and we don't have to rely on PGO.
Well, I see "we", I mean the ANGLE-4-Android folks.
Shahbaz YoussefiSaying "we care about perf so much that we need an opt-out for a thing that has no measurable cost" and "we don't use PGO" is IMHO not a very convincing position.
I'd say we land the flag without an opt-out for now, and add it if there's data that shows that we actually need it.
Nico WeberI never agreed it doesn't have a measurable cost :)
You haven't shown data that it does though, while Kalvin has data showing that it doesn't.
You haven't shown data
I could have someone gather some data if you have instructions.
Pending said instructions.
Shahbaz YoussefiWould it be acceptable if we remove the "//build/config/compiler:sanitize_c_array_bounds" config in standalone ANGLE, like we do for others here https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/gni/angle.gni;l=352;drc=3c2bff4b540d1c98b6dd4ea901b05a3b7e6112b8?
Yuly NovikovYes sure, that would be great!
Nico WeberOK, so the plan is:
1. Abandon this
2. Land https://chromium-review.googlesource.com/c/chromium/src/+/6600050
3. Roll Chromium into ANGLE
4. Disable "//build/config/compiler:sanitize_c_array_bounds" config in ANGLE.
Ideally we disable the config in the roll, but I think it's not a big deal if we do it a bit later, since rolling and landing would be hard to synchronize.
Yuly NovikovPlease only do 4 if you have numbers to back up it having an effect (in particular, with PGO enabled). We likely do want this on in angle as it ships in chromium.
Nico WeberIt would be enabled in the version of ANGLE that's built as a part of Chromium, and would be disabled in standalone ANGLE, which is shipped elsewhere.
Win-Win!
Sure, works for me from a chrome PoV.
Who are contacts on the android gles driver team who can explicitly confirm that they're not interested in security hardening measures without demonstrated performance overhead? That's fairly different from what I've heard from my contacts in android land. I'd like to check with both the gles driver team and my contacts and make sure they're aligned on this.
Perhaps @abdolr...@google.com could take a look at driver_overhead2 and some other cpu-perf sensitive traces? Again, needs instructions (I'm not sure how we'd get from https://chromium-review.googlesource.com/c/chromium/src/+/6600050 to ANGLE standalone using it, before it all lands).
security hardening measures
It's all about the context. Chrome serves multiple GL "users" (the different pages) that can't trust each other. So yes, of course it needs to be as secure as possible. On the other hand, a mobile game doesn't need this kind of security. What it does need is longer battery life.
Related details
- Kalvin Lee
- Nico Weber
- Yuly Novikov
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuly Novikov (Gerrit)
Yuly Novikov added 1 comment![]( "Open in Gerrit")
Shahbaz YoussefiWould it be acceptable if we remove the "//build/config/compiler:sanitize_c_array_bounds" config in standalone ANGLE, like we do for others here https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/gni/angle.gni;l=352;drc=3c2bff4b540d1c98b6dd4ea901b05a3b7e6112b8?
Yuly NovikovYes sure, that would be great!
Nico WeberOK, so the plan is:
1. Abandon this
2. Land https://chromium-review.googlesource.com/c/chromium/src/+/6600050
3. Roll Chromium into ANGLE
4. Disable "//build/config/compiler:sanitize_c_array_bounds" config in ANGLE.
Ideally we disable the config in the roll, but I think it's not a big deal if we do it a bit later, since rolling and landing would be hard to synchronize.
Yuly NovikovPlease only do 4 if you have numbers to back up it having an effect (in particular, with PGO enabled). We likely do want this on in angle as it ships in chromium.
Nico WeberIt would be enabled in the version of ANGLE that's built as a part of Chromium, and would be disabled in standalone ANGLE, which is shipped elsewhere.
Win-Win!
Shahbaz YoussefiSure, works for me from a chrome PoV.
Who are contacts on the android gles driver team who can explicitly confirm that they're not interested in security hardening measures without demonstrated performance overhead? That's fairly different from what I've heard from my contacts in android land. I'd like to check with both the gles driver team and my contacts and make sure they're aligned on this.
Perhaps @abdolr...@google.com could take a look at driver_overhead2 and some other cpu-perf sensitive traces? Again, needs instructions (I'm not sure how we'd get from https://chromium-review.googlesource.com/c/chromium/src/+/6600050 to ANGLE standalone using it, before it all lands).
security hardening measures
It's all about the context. Chrome serves multiple GL "users" (the different pages) that can't trust each other. So yes, of course it needs to be as secure as possible. On the other hand, a mobile game doesn't need this kind of security. What it does need is longer battery life.
I'm not sure how we'd get from https://chromium-review.googlesource.com/c/chromium/src/+/6600050 to ANGLE standalone using it, before it all lands
Seems as simple as adding these to `cflags`:
```
"-fsanitize=array-bounds",
"-fsanitize-trap=array-bounds",
```
We could do it e.g. here: https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/BUILD.gn;l=471;drc=3c2bff4b540d1c98b6dd4ea901b05a3b7e6112b8
Related details
- Kalvin Lee
- Nico Weber
- Shahbaz Youssefi
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kalvin Lee (Gerrit)
Kalvin Lee abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |