Roll abseil_revision de672d07fe..09e52eb265 [chromium/src : main]
Danil Chapovalov (Gerrit)
Danil Chapovalov added 1 comment![]( "Open in Gerrit")
Richard, can you please clobber usage.
Related details
- Mirko Bonadei
- Richard Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mirko Bonadei (Gerrit)
Mirko Bonadei voted Code-Review+1![]( "Open in Gerrit")
- Danil Chapovalov
- Richard Wang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Richard Wang (Gerrit)
Richard Wang added 1 comment![]( "Open in Gerrit")
Richard, can you please clobber usage.
Hi Danil, I'd love to be able to help but I'm unfortunately unfamiliar with the ask and its consequences.
If this is not too urgent https://chromium.googlesource.com/website/+/HEAD/site/Home/chromium-clobber-landmines/index.md lists iannucci@ for clobbering related queries, I've pinged my team to see if anyone else is more familiar with what's being asked and can jump in and/or fill me in on what this is.
Note it's EoD local timezone here but hopefully someone more familiar can jump in if this is urgent.
Thanks!
Related details
- Danil Chapovalov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Chapovalov (Gerrit)
Danil Chapovalov added 1 comment![]( "Open in Gerrit")
Richard WangRichard, can you please clobber usage.
Hi Danil, I'd love to be able to help but I'm unfortunately unfamiliar with the ask and its consequences.
If this is not too urgent https://chromium.googlesource.com/website/+/HEAD/site/Home/chromium-clobber-landmines/index.md lists iannucci@ for clobbering related queries, I've pinged my team to see if anyone else is more familiar with what's being asked and can jump in and/or fill me in on what this is.
Note it's EoD local timezone here but hopefully someone more familiar can jump in if this is urgent.
Thanks!
Thank you!, it is not too urgent and can easily wait until US wakes up,
I'll add iannucci@
Related details
- Robbie Iannucci
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Chapovalov (Gerrit)
Danil Chapovalov added 1 comment![]( "Open in Gerrit")
- Hans Wennborg
- Robbie Iannucci
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hans Wennborg (Gerrit)
Hans Wennborg added 1 comment![]( "Open in Gerrit")
Hans, are you familiar with clobber?
Yes, and my understanding is that we try not to use it unless absolutely necessary since it's disruptive/expensive.
Looking at the linked bug, it doesn't seem like the Dawn issue is well understood yet.
Related details
- Danil Chapovalov
- Robbie Iannucci
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Harrison (Gerrit)
Ryan Harrison added 1 comment![]( "Open in Gerrit")
The need to clobber is a bug in the abseil Build rules.
The missing symbol, absl::container_internal::ReserveEmptyNonAllocatedTableToFitBucketCount, was removed in this CL, but because in third_party/abseil-cpp/absl/container/BUILD.gn, things that depend raw_hash_set and raw_hash_map are mistakenly listing them as deps instead of public_deps, which means they don't propegate correctly to the external targets, like content_shell and the dawn wire fuzzer, so when SISO goes to build it doesn't know it needs to rebuild those targets too.
Related details
- Danil Chapovalov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Chapovalov (Gerrit)
Danil Chapovalov added 1 comment![]( "Open in Gerrit")
The need to clobber is a bug in the abseil Build rules.
The missing symbol, absl::container_internal::ReserveEmptyNonAllocatedTableToFitBucketCount, was removed in this CL, but because in third_party/abseil-cpp/absl/container/BUILD.gn, things that depend raw_hash_set and raw_hash_map are mistakenly listing them as deps instead of public_deps, which means they don't propegate correctly to the external targets, like content_shell and the dawn wire fuzzer, so when SISO goes to build it doesn't know it needs to rebuild those targets too.
Can you please help me understand why siso needs it to be public_deps?
Reading the gn documentation, that not how I understand the difference between `deps` and `public_deps`:
they are exposed as deps not public_deps because they do not allow to include implementation details headers.
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_deps
“Private dependencies are
propagated up the dependency tree and linked to dependent targets, but do not
grant the ability to include headers from the dependency.”
Documentation for public_deps
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_public_deps
also doesn't hint why rebuilding requires dependencies to be public.
(meanwhile I'll try to mark them as public_deps to check if that helps)
Related details
- Ryan Harrison
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Chapovalov (Gerrit)
Danil Chapovalov added 1 comment![]( "Open in Gerrit")
Danil ChapovalovThe need to clobber is a bug in the abseil Build rules.
The missing symbol, absl::container_internal::ReserveEmptyNonAllocatedTableToFitBucketCount, was removed in this CL, but because in third_party/abseil-cpp/absl/container/BUILD.gn, things that depend raw_hash_set and raw_hash_map are mistakenly listing them as deps instead of public_deps, which means they don't propegate correctly to the external targets, like content_shell and the dawn wire fuzzer, so when SISO goes to build it doesn't know it needs to rebuild those targets too.
Can you please help me understand why siso needs it to be public_deps?
Reading the gn documentation, that not how I understand the difference between `deps` and `public_deps`:
they are exposed as deps not public_deps because they do not allow to include implementation details headers.
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_deps
“Private dependencies are
propagated up the dependency tree and linked to dependent targets, but do not
grant the ability to include headers from the dependency.”Documentation for public_deps
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_public_deps
also doesn't hint why rebuilding requires dependencies to be public.(meanwhile I'll try to mark them as public_deps to check if that helps)
In patchset#9 I've replaced deps with public_deps, but linking error is exactly the same as before (e.g. in patchset#8)
Related details
- Ryan Harrison
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Harrison (Gerrit)
Ryan Harrison added 1 comment![]( "Open in Gerrit")
Danil ChapovalovThe need to clobber is a bug in the abseil Build rules.
The missing symbol, absl::container_internal::ReserveEmptyNonAllocatedTableToFitBucketCount, was removed in this CL, but because in third_party/abseil-cpp/absl/container/BUILD.gn, things that depend raw_hash_set and raw_hash_map are mistakenly listing them as deps instead of public_deps, which means they don't propegate correctly to the external targets, like content_shell and the dawn wire fuzzer, so when SISO goes to build it doesn't know it needs to rebuild those targets too.
Can you please help me understand why siso needs it to be public_deps?
Reading the gn documentation, that not how I understand the difference between `deps` and `public_deps`:
they are exposed as deps not public_deps because they do not allow to include implementation details headers.
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_deps
“Private dependencies are
propagated up the dependency tree and linked to dependent targets, but do not
grant the ability to include headers from the dependency.”Documentation for public_deps
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_public_deps
also doesn't hint why rebuilding requires dependencies to be public.(meanwhile I'll try to mark them as public_deps to check if that helps)
(This is a bit based on my recollections of maintaining the Dawn version of the Abseil GN rules, so a touch fuzzy, and the gory details of how GN works are not my area of expertise)
The issue is deps doesn't propegate up the needed for the change in :raw_set, which would be fine if the change wasn't visible, but the issue is that these target's .h files are including raw_set.h directly in them, so raw_set.h is part of the API they are exposing effectively, but Siso/ninja doesn't think that things like liks :content_shell need to be rebuilt, so you end up with the cached version of which the old removed symbols in it.
My understanding of how this works at a high level.
An end using target like the dawn wire fuzzer (:W) includes dependency from abseil (:A), and includes the A.h header file access the API. :A depends on :raw_set, but A.h directly includes raw_set.h, which means that :W now has an indirect dependency on :raw_set, but deps is saying the :raw_set is a private dependency of :A, so if :raw_set changes, but :A doesn't, ninja thinkgs it just needs to rebuild :raw_set and :A, but not :W, because the api surface of :A hasn't changed. public_deps is basically saying that :raw_set is part of the API surface of :A that might change, so if :raw_set changes that means anything that depends on :A, like :W, will also need a rebuild.
Related details
- Danil Chapovalov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Harrison (Gerrit)
Ryan Harrison added 1 comment![]( "Open in Gerrit")
Danil ChapovalovThe need to clobber is a bug in the abseil Build rules.
The missing symbol, absl::container_internal::ReserveEmptyNonAllocatedTableToFitBucketCount, was removed in this CL, but because in third_party/abseil-cpp/absl/container/BUILD.gn, things that depend raw_hash_set and raw_hash_map are mistakenly listing them as deps instead of public_deps, which means they don't propegate correctly to the external targets, like content_shell and the dawn wire fuzzer, so when SISO goes to build it doesn't know it needs to rebuild those targets too.
Ryan HarrisonCan you please help me understand why siso needs it to be public_deps?
Reading the gn documentation, that not how I understand the difference between `deps` and `public_deps`:
they are exposed as deps not public_deps because they do not allow to include implementation details headers.
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_deps
“Private dependencies are
propagated up the dependency tree and linked to dependent targets, but do not
grant the ability to include headers from the dependency.”Documentation for public_deps
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_public_deps
also doesn't hint why rebuilding requires dependencies to be public.(meanwhile I'll try to mark them as public_deps to check if that helps)
(This is a bit based on my recollections of maintaining the Dawn version of the Abseil GN rules, so a touch fuzzy, and the gory details of how GN works are not my area of expertise)
The issue is deps doesn't propegate up the needed for the change in :raw_set, which would be fine if the change wasn't visible, but the issue is that these target's .h files are including raw_set.h directly in them, so raw_set.h is part of the API they are exposing effectively, but Siso/ninja doesn't think that things like liks :content_shell need to be rebuilt, so you end up with the cached version of which the old removed symbols in it.
My understanding of how this works at a high level.
An end using target like the dawn wire fuzzer (:W) includes dependency from abseil (:A), and includes the A.h header file access the API. :A depends on :raw_set, but A.h directly includes raw_set.h, which means that :W now has an indirect dependency on :raw_set, but deps is saying the :raw_set is a private dependency of :A, so if :raw_set changes, but :A doesn't, ninja thinkgs it just needs to rebuild :raw_set and :A, but not :W, because the api surface of :A hasn't changed. public_deps is basically saying that :raw_set is part of the API surface of :A that might change, so if :raw_set changes that means anything that depends on :A, like :W, will also need a rebuild.
I apologize, there is a bit of a threading issue here, my response was written before I say that public_deps didn't resolve the issue
Related details
- Danil Chapovalov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Chapovalov (Gerrit)
Danil Chapovalov added 1 comment![]( "Open in Gerrit")
Danil ChapovalovThe need to clobber is a bug in the abseil Build rules.
The missing symbol, absl::container_internal::ReserveEmptyNonAllocatedTableToFitBucketCount, was removed in this CL, but because in third_party/abseil-cpp/absl/container/BUILD.gn, things that depend raw_hash_set and raw_hash_map are mistakenly listing them as deps instead of public_deps, which means they don't propegate correctly to the external targets, like content_shell and the dawn wire fuzzer, so when SISO goes to build it doesn't know it needs to rebuild those targets too.
Ryan HarrisonCan you please help me understand why siso needs it to be public_deps?
Reading the gn documentation, that not how I understand the difference between `deps` and `public_deps`:
they are exposed as deps not public_deps because they do not allow to include implementation details headers.
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_deps
“Private dependencies are
propagated up the dependency tree and linked to dependent targets, but do not
grant the ability to include headers from the dependency.”Documentation for public_deps
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_public_deps
also doesn't hint why rebuilding requires dependencies to be public.(meanwhile I'll try to mark them as public_deps to check if that helps)
Ryan Harrison(This is a bit based on my recollections of maintaining the Dawn version of the Abseil GN rules, so a touch fuzzy, and the gory details of how GN works are not my area of expertise)
The issue is deps doesn't propegate up the needed for the change in :raw_set, which would be fine if the change wasn't visible, but the issue is that these target's .h files are including raw_set.h directly in them, so raw_set.h is part of the API they are exposing effectively, but Siso/ninja doesn't think that things like liks :content_shell need to be rebuilt, so you end up with the cached version of which the old removed symbols in it.
My understanding of how this works at a high level.
An end using target like the dawn wire fuzzer (:W) includes dependency from abseil (:A), and includes the A.h header file access the API. :A depends on :raw_set, but A.h directly includes raw_set.h, which means that :W now has an indirect dependency on :raw_set, but deps is saying the :raw_set is a private dependency of :A, so if :raw_set changes, but :A doesn't, ninja thinkgs it just needs to rebuild :raw_set and :A, but not :W, because the api surface of :A hasn't changed. public_deps is basically saying that :raw_set is part of the API surface of :A that might change, so if :raw_set changes that means anything that depends on :A, like :W, will also need a rebuild.
I apologize, there is a bit of a threading issue here, my response was written before I say that public_deps didn't resolve the issue
btw, the reason I suspect dawn build rules is that absl flat_hash_map is very popular header, but I've only seen errors with linking dawn targets.
Related details
- Ryan Harrison
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Harrison (Gerrit)
Ryan Harrison added 1 comment![]( "Open in Gerrit")
Danil ChapovalovThe need to clobber is a bug in the abseil Build rules.
The missing symbol, absl::container_internal::ReserveEmptyNonAllocatedTableToFitBucketCount, was removed in this CL, but because in third_party/abseil-cpp/absl/container/BUILD.gn, things that depend raw_hash_set and raw_hash_map are mistakenly listing them as deps instead of public_deps, which means they don't propegate correctly to the external targets, like content_shell and the dawn wire fuzzer, so when SISO goes to build it doesn't know it needs to rebuild those targets too.
Ryan HarrisonCan you please help me understand why siso needs it to be public_deps?
Reading the gn documentation, that not how I understand the difference between `deps` and `public_deps`:
they are exposed as deps not public_deps because they do not allow to include implementation details headers.
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_deps
“Private dependencies are
propagated up the dependency tree and linked to dependent targets, but do not
grant the ability to include headers from the dependency.”Documentation for public_deps
https://gn.googlesource.com/gn/+/HEAD/docs/reference.md#var_public_deps
also doesn't hint why rebuilding requires dependencies to be public.(meanwhile I'll try to mark them as public_deps to check if that helps)
Ryan Harrison(This is a bit based on my recollections of maintaining the Dawn version of the Abseil GN rules, so a touch fuzzy, and the gory details of how GN works are not my area of expertise)
The issue is deps doesn't propegate up the needed for the change in :raw_set, which would be fine if the change wasn't visible, but the issue is that these target's .h files are including raw_set.h directly in them, so raw_set.h is part of the API they are exposing effectively, but Siso/ninja doesn't think that things like liks :content_shell need to be rebuilt, so you end up with the cached version of which the old removed symbols in it.
My understanding of how this works at a high level.
An end using target like the dawn wire fuzzer (:W) includes dependency from abseil (:A), and includes the A.h header file access the API. :A depends on :raw_set, but A.h directly includes raw_set.h, which means that :W now has an indirect dependency on :raw_set, but deps is saying the :raw_set is a private dependency of :A, so if :raw_set changes, but :A doesn't, ninja thinkgs it just needs to rebuild :raw_set and :A, but not :W, because the api surface of :A hasn't changed. public_deps is basically saying that :raw_set is part of the API surface of :A that might change, so if :raw_set changes that means anything that depends on :A, like :W, will also need a rebuild.
I apologize, there is a bit of a threading issue here, my response was written before I say that public_deps didn't resolve the issue
The other possibility that I was considering was that it was an issue with dawn including internal abseil targets directly, but I am pretty sure that dawn just uses the top-level public :abseil target now and the build in Chromium would be complaining if dawn was using them, since there is an allowlist for reaching into the internals of Dawn.
Given that we are seeing this issue with both dawn and non-targets (like :content_shell) and Dawn doesn't reference the changed header, raw_hash_set.h, directly, that this is some sort of dependency propegation issue.
Ryan Harrison (Gerrit)
Didn't it also fail on :content_shell for CrOS, https://ci.chromium.org/ui/p/chrome/builders/try/linux-chromeos-compile-chrome/1532688 or am I misattributing that?
Related details
- Danil Chapovalov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Chapovalov (Gerrit)
Danil Chapovalov added 1 comment![]( "Open in Gerrit")
It did, but it failed to link dawn file inthere too:
```
[37698/58776] LINK ./content_shell
python3 ../../build/toolchain/gcc_link_wrapper.py --output=./content_shell -- ld.lld: error: undefined symbol: absl::container_internal::ReserveEmptyNonAllocatedTableToFitBucketCount(absl::container_internal::CommonFields&, absl::container_internal::PolicyFunctions const&, unsigned long)
>>> referenced by raw_hash_set.h:2506 (../../third_party/abseil-cpp/absl/container/internal/raw_hash_set.h:2506)
>>> obj/third_party/**dawn**/src/dawn/native/sources/Instance.o:
```
Related details
- Ryan Harrison
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Harrison (Gerrit)
Ryan Harrison added 1 comment![]( "Open in Gerrit")
I agree it is something to do with cache'ing Dawn's build target incorrectly, but I am really not sure why public_deps isn't solving the issus.
Re-reading the docs, one of the major differences between deps and public_deps, is that deps doesn't expose the headers of the dependency transitively, whereas public_deps does, which really what seems like the problem here to me, is that Instance.h is including "absl/container/flat_hash_set.h", which in turn is including "absl/container/internal/raw_hash_map.h", so when using deps this will mean that if "absl/container/internal/raw_hash_map.h" changes GN won't think Instance.h and thus Instance.cc needs to be rebuilt, which would explain the error of including a symbol that was removed.
I really don't get why that doesn't fix the issue. I am a little suspicious of the '// IWYU pragma: export' since that is pre-C++20 Clang way of forcing a symbol to be exported, which suggests to me there has been linking issues here before.
flat_hash_map.h is a header only implementation, so we cannot refactor it to push the inclusion down into a .cc to break the chain.
Related details
- Danil Chapovalov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Harrison (Gerrit)
flat_hash_set.h is a pretty heavily used header in Chromium, so is Dawn holding it wrong as it were when depending on :absl?
The only significant difference I can see is that Dawn puts :absl in a group as a public_dep so that we can have one place in the code base where were have to use "${dawn_abseil_dir}:absl" to allow support for abseil not being in the 'standard' chromium location, which is needed by Skia
Ryan Harrison (Gerrit)
Since dawn_root and dawn_abseil_dir are both defined in the same .gni file, they should be available in all of the same places the Dawn GN files, so I could speculatively remove the group and replace it with a bare reference to :absl
Ryan Harrison (Gerrit)
From the docs I think it shouldn't be the group wrapper, it seems like public_deps should be transitively handled. There are a couple of places in Dawn where the dependency is being used as a dep instead of a public_dep that might be borking up the dependency graph? I can send up a patch to fix these, but that would be in Dawn, so would need to land there and roll before you could access it.
I am honestly grasping at straws. I am not sure if there is someone with more familiarity with GN/Abseil that should be looped in here.
Ryan Harrison (Gerrit)
I have written https://dawn-review.googlesource.com/c/dawn/+/322835 to fix potential issues I have identified on the Dawn side.
Danil Chapovalov (Gerrit)
Danil Chapovalov added 1 comment![]( "Open in Gerrit")
Hans WennborgHans, are you familiar with clobber?
Yes, and my understanding is that we try not to use it unless absolutely necessary since it's disruptive/expensive.
Looking at the linked bug, it doesn't seem like the Dawn issue is well understood yet.
Thank you for the feedback, will not use global clobber then.
Will continue search for the root cause and halt import until it is understood.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Chapovalov (Gerrit)
Danil Chapovalov removed Hans Wennborg from this change![]( "Open in Gerrit")
- Hans Wennborg
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Chapovalov (Gerrit)
Danil Chapovalov removed Richard Wang from this change![]( "Open in Gerrit")
- Danil Chapovalov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |