New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 899771 link

Starred by 1 user
Status: Assigned
Owner:
tomfinegan@google.com
Cc:
█ johannkoenig@chromium.org
█ tomfinegan@chromium.org
jzern@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 898837



Sign in to add a comment

third_party/libaom is not gn check clean (include headers in the wrong target)

Project Member Reported by brat...@opera.com, Oct 29 
TO REPRODUCE
------------
0. Have a product configured in out/Default/args.gn which includes libaom
1. Run gn check out/Default "//third_party/libaom/*"

EXPECTED
---------
gn check to say that everything is fine.


ACTUALLY
---------
gn check says that there are illegal includes related to the following headers:
av1/common/x86/av1_txfm_sse2.h
av1/common/x86/av1_inv_txfm_ssse3.h
aom_dsp/x86/blend_mask_sse4.h
aom_dsp/x86/convolve.h
aom_dsp/x86/convolve_sse2.h
aom_dsp/x86/lpf_common_sse2.h
aom_dsp/x86/transpose_sse2.h
aom_dsp/x86/txfm_common_sse2.h


This is because the headers above are listed in a particular sub-target but are used in many sub-targets (not just sse2).

The build scripts are generated which makes it non-trivial to fix, but if something like the below could be generated, it would work:

diff --git a/third_party/libaom/BUILD.gn b/third_party/libaom/BUILD.gn
index 2a4cde7..2e4132d 100644
--- a/third_party/libaom/BUILD.gn
+++ b/third_party/libaom/BUILD.gn
@@ -84,6 +84,10 @@ if (enable_av1_decoder) {
     # link time. On Mac, this results in undefined references to the intrinsic
     # symbols.
 
+    source_set("libaom_intrinsics_x86_shared_headers") {
+      sources = aom_dsp_common_intrin_x86_shared_headers + aom_av1_common_intrin_x86_shared_headers
+    }
+
     source_set("libaom_intrinsics_sse2") {
       configs -= [ "//build/config/compiler:chromium_code" ]
       configs += [ "//build/config/compiler:no_chromium_code" ]
@@ -93,6 +97,7 @@ if (enable_av1_decoder) {
       }
       sources = aom_av1_common_intrin_sse2
       sources += aom_dsp_common_intrin_sse2
+      deps = [ ":libaom_intrinsics_x86_shared_headers" ]
     }
 
     source_set("libaom_intrinsics_ssse3") {
@@ -104,6 +109,7 @@ if (enable_av1_decoder) {
       }
       sources = aom_av1_common_intrin_ssse3
       sources += aom_dsp_common_intrin_ssse3
+      deps = [ ":libaom_intrinsics_x86_shared_headers" ]
     }
 
     source_set("libaom_intrinsics_sse4_1") {
@@ -115,6 +121,7 @@ if (enable_av1_decoder) {
       }
       sources = aom_av1_common_intrin_sse4_1
       sources += aom_dsp_common_intrin_sse4_1
+      deps = [ ":libaom_intrinsics_x86_shared_headers" ]
     }
 
     source_set("libaom_intrinsics_sse4_2") {
@@ -124,6 +131,7 @@ if (enable_av1_decoder) {
       if (!is_win || is_clang) {
         cflags = [ "-msse4.2" ]
       }
+      deps = [ ":libaom_intrinsics_x86_shared_headers" ]
     }
 
     source_set("libaom_intrinsics_avx") {
@@ -135,6 +143,7 @@ if (enable_av1_decoder) {
       } else {
         cflags = [ "-mavx" ]
       }
+      deps = [ ":libaom_intrinsics_x86_shared_headers" ]
     }
 
     source_set("libaom_intrinsics_avx2") {
@@ -148,6 +157,7 @@ if (enable_av1_decoder) {
       }
       sources = aom_av1_common_intrin_avx2
       sources += aom_dsp_common_intrin_avx2
+      deps = [ ":libaom_intrinsics_x86_shared_headers" ]
     }
   }
 
@@ -199,6 +209,12 @@ if (enable_av1_decoder) {
         cpu_arch_full == "arm-neon-cpu-detect") {
       deps += [ ":libaom_intrinsics_neon" ]
     }
+
+    # Everything in deps up to this point is internal sub-targets so
+    # we can just signal that everything in deps can use headers from
+    # the main build target.
+    allow_circular_includes_from = deps
+
     if (is_android) {
       deps += [ "//third_party/android_tools:cpu_features" ]
     }
diff --git a/third_party/libaom/libaom_srcs.gni b/third_party/libaom/libaom_srcs.gni
index 1cf7150..4195d1b 100644
--- a/third_party/libaom/libaom_srcs.gni
+++ b/third_party/libaom/libaom_srcs.gni
@@ -1,5 +1,10 @@
 # This file is generated. DO NOT EDIT.
 
+aom_av1_common_intrin_x86_shared_headers = [
+  "//third_party/libaom/source/libaom/av1/common/x86/av1_txfm_sse2.h",
+  "//third_party/libaom/source/libaom/av1/common/x86/av1_inv_txfm_ssse3.h",
+]
+
 aom_av1_common_intrin_avx2 = [
   "//third_party/libaom/source/libaom/av1/common/cdef_block_avx2.c",
   "//third_party/libaom/source/libaom/av1/common/x86/av1_inv_txfm_avx2.c",
@@ -44,7 +49,6 @@ aom_av1_common_intrin_sse2 = [
   "//third_party/libaom/source/libaom/av1/common/x86/highbd_convolve_2d_sse2.c",
   "//third_party/libaom/source/libaom/av1/common/x86/jnt_convolve_sse2.c",
   "//third_party/libaom/source/libaom/av1/common/x86/wiener_convolve_sse2.c",
-  "//third_party/libaom/source/libaom/av1/common/x86/av1_txfm_sse2.h",
 ]
 
 aom_av1_common_intrin_sse4_1 = [
@@ -68,7 +72,6 @@ aom_av1_common_intrin_sse4_1 = [
 aom_av1_common_intrin_ssse3 = [
   "//third_party/libaom/source/libaom/av1/common/cdef_block_ssse3.c",
   "//third_party/libaom/source/libaom/av1/common/x86/av1_inv_txfm_ssse3.c",
-  "//third_party/libaom/source/libaom/av1/common/x86/av1_inv_txfm_ssse3.h",
   "//third_party/libaom/source/libaom/av1/common/x86/cfl_ssse3.c",
   "//third_party/libaom/source/libaom/av1/common/x86/highbd_convolve_2d_ssse3.c",
   "//third_party/libaom/source/libaom/av1/common/x86/highbd_wiener_convolve_ssse3.c",
@@ -382,28 +385,31 @@ aom_dsp_common_intrin_neon = [
   "//third_party/libaom/source/libaom/aom_dsp/arm/blend_a64_mask_neon.c",
 ]
 
+aom_dsp_common_intrin_x86_shared_headers = [
+  "//third_party/libaom/source/libaom/aom_dsp/x86/blend_mask_sse4.h",
+  "//third_party/libaom/source/libaom/aom_dsp/x86/convolve.h",
+  "//third_party/libaom/source/libaom/aom_dsp/x86/convolve_sse2.h",
+  "//third_party/libaom/source/libaom/aom_dsp/x86/lpf_common_sse2.h",
+  "//third_party/libaom/source/libaom/aom_dsp/x86/transpose_sse2.h",
+  "//third_party/libaom/source/libaom/aom_dsp/x86/txfm_common_sse2.h",
+]
+
 aom_dsp_common_intrin_sse2 = [
   "//third_party/libaom/source/libaom/aom_dsp/x86/aom_subpixel_8t_intrin_sse2.c",
   "//third_party/libaom/source/libaom/aom_dsp/x86/aom_asm_stubs.c",
-  "//third_party/libaom/source/libaom/aom_dsp/x86/convolve.h",
-  "//third_party/libaom/source/libaom/aom_dsp/x86/convolve_sse2.h",
   "//third_party/libaom/source/libaom/aom_dsp/x86/fft_sse2.c",
   "//third_party/libaom/source/libaom/aom_dsp/x86/highbd_convolve_sse2.c",
   "//third_party/libaom/source/libaom/aom_dsp/x86/highbd_intrapred_sse2.c",
   "//third_party/libaom/source/libaom/aom_dsp/x86/highbd_loopfilter_sse2.c",
   "//third_party/libaom/source/libaom/aom_dsp/x86/intrapred_sse2.c",
   "//third_party/libaom/source/libaom/aom_dsp/x86/loopfilter_sse2.c",
-  "//third_party/libaom/source/libaom/aom_dsp/x86/lpf_common_sse2.h",
   "//third_party/libaom/source/libaom/aom_dsp/x86/mem_sse2.h",
-  "//third_party/libaom/source/libaom/aom_dsp/x86/transpose_sse2.h",
-  "//third_party/libaom/source/libaom/aom_dsp/x86/txfm_common_sse2.h",
   "//third_party/libaom/source/libaom/aom_dsp/x86/sum_squares_sse2.h",
   "//third_party/libaom/source/libaom/aom_dsp/x86/avg_intrin_sse2.c",
   "//third_party/libaom/source/libaom/aom_dsp/x86/bitdepth_conversion_sse2.h",
 ]
 
 aom_dsp_common_intrin_sse4_1 = [
-  "//third_party/libaom/source/libaom/aom_dsp/x86/blend_mask_sse4.h",
   "//third_party/libaom/source/libaom/aom_dsp/x86/blend_a64_hmask_sse4.c",
   "//third_party/libaom/source/libaom/aom_dsp/x86/blend_a64_mask_sse4.c",
   "//third_party/libaom/source/libaom/aom_dsp/x86/blend_a64_vmask_sse4.c",

Comment 1 by jaikk@google.com, Nov 1

Owner: tomfinegan@chromium.org
Status: Assigned (was: Untriaged)

Comment 2 by tomfinegan@google.com, Nov 1

Owner: tomfinegan@google.com
@bratell:

Just so that I actually understand what's going on: What's gn really complaining about here? 

Is it that the reported files are included by source files in targets that do not have explicit dependencies on the target where the include files are actually referenced?

Comment 3 by brat...@opera.com, Nov 2 

I can paste a big chunk of text here but it's easier if you just run

gn check out/Default "//third_party/libaom/*"

(where out/Default might be something else in your tree)

The problem is that the sub build targets include headers from their sibling build targets without any explicit dependency. SSE3 files include sse2 headers and av1 headers include sse4 headers and so on.

Since there should not be any build dependencies (mutually exclusive targets), either the includes have to go (code refactoring) or the headers have to be moved to a common build target. The patch I listed above did the latter, but larger changes with new shared headers might be cleaner.

Comment 4 by brat...@opera.com, Nov 2 

https://pastebin.com/54tZ7e8z has the error output on my computer. It's 1200 lines long, but it's all about the same 8 header files used from the "wrong" build targets.

Comment 5 by johannko...@google.com, Nov 2 

Would it be reasonable to include the headers in the very top target? I imagine we don't get warnings for headers like:
aom_ports/mem.h
aom/aom_integer.h
aom_dsp/aom_dsp_common.h
etc

Including all headers for all architectures at the top *should* be OK since having, for example, aom_dsp/x86/obmc_intrinsic_ssse3.h in the file list for the neon build shouldn't trigger any errors because it shouldn't be processed.

Comment 6 by brat...@opera.com, Nov 2 

There are warnings for those too. I forgot to explain that part. The patch above also contains a line:

+    allow_circular_includes_from = deps

Which tells gn that all sub build targets can include headers from the root ("libaom") build target. The allow_circular_includes_from part is needed, but after that moving the common headers to aom_dsp_common_sources seems like a simple fix indeed!
►

Sign in to add a comment