Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2515503002:
Disable thumb when building tcmalloc (Closed)
DescriptionDisable thumb when building tcmalloc
This fixes the arm/linux build which is currently
broken because third_party/tc_malloc is not currently
buildable with -mthumb:
atomicops-internals-arm-v6plus.h:268:8:
error: instruction requires: arm-mode "strexd %0, %2, [%3]\n"`
BUG=564059
Committed: https://crrev.com/446886fca650e9239999df634ef6bc890b95680d
Cr-Commit-Position: refs/heads/master@{#433052}
Patch Set 1 #
Total comments: 1
Patch Set 2 : . #
Messages
Total messages: 41 (11 generated)
Description was changed from ========== Disable thumb when building tcmalloc This fixes the arm/linux build which is currently broken because third_party/tc_malloc is not currently buildable with -mthumb: atomicops-internals-arm-v6plus.h:268:8: error: instruction requires: arm-mode "strexd %0, %2, [%3]\n"` BUG= ========== to ========== Disable thumb when building tcmalloc This fixes the arm/linux build which is currently broken because third_party/tc_malloc is not currently buildable with -mthumb: atomicops-internals-arm-v6plus.h:268:8: error: instruction requires: arm-mode "strexd %0, %2, [%3]\n"` BUG=564059 ==========
sbc@chromium.org changed reviewers: + dpranke@chromium.org, thakis@chromium.org
dpranke@chromium.org changed reviewers: + brettw@chromium.org
lgtm. You'll need brettw@'s OWNERS approval as well.
I'm a bit confused looking at the root build configuration. It's my understanding that mixing thumb and non-thum requires the -thumb-interwork flag to be set. But in build\config\compiler\BUILD.gn I see this only getting set when it's android and not clang. What does disabling thumb on a per-target basis mean in the context of this?
On 2016/11/17 21:28:31, brettw (ping on IM after 24h) wrote:
> I'm a bit confused looking at the root build configuration. It's my
> understanding that mixing thumb and non-thum requires the -thumb-interwork
flag
> to be set. But in build\config\compiler\BUILD.gn I see this only getting set
> when it's android and not clang. What does disabling thumb on a per-target
basis
> mean in the context of this?
Good question. I had a hunch that this just worked (IIRC it works by setting
the low order bit of the function address for thumb function, i.e. all thumb
calls are to odd-numbered addresses and arm calls are to even numbered
addresses) and the man page seems to confirm this:
-mthumb-interwork
Generate code that supports calling between the ARM and Thumb
instruction sets. Without
this option, on pre-v5 architectures, the two instruction sets cannot
be reliably used
inside one program. The default is -mno-thumb-interwork, since
slightly larger code is
generated when -mthumb-interwork is specified. In AAPCS
configurations this option is
meaningless.
Also, I'm fairly sure the linker complains if you try to link arm and thumb
functions in way that won't work at runtime.
lgtm https://codereview.chromium.org/2515503002/diff/1/base/allocator/BUILD.gn File base/allocator/BUILD.gn (right): https://codereview.chromium.org/2515503002/diff/1/base/allocator/BUILD.gn#new... base/allocator/BUILD.gn:221: "//build/config/compiler:compiler_arm_thumb", Can you comment here why this is removed?
The CQ bit was checked by sbc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sbc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/2515503002/#ps20001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Disable thumb when building tcmalloc This fixes the arm/linux build which is currently broken because third_party/tc_malloc is not currently buildable with -mthumb: atomicops-internals-arm-v6plus.h:268:8: error: instruction requires: arm-mode "strexd %0, %2, [%3]\n"` BUG=564059 ========== to ========== Disable thumb when building tcmalloc This fixes the arm/linux build which is currently broken because third_party/tc_malloc is not currently buildable with -mthumb: atomicops-internals-arm-v6plus.h:268:8: error: instruction requires: arm-mode "strexd %0, %2, [%3]\n"` BUG=564059 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Disable thumb when building tcmalloc This fixes the arm/linux build which is currently broken because third_party/tc_malloc is not currently buildable with -mthumb: atomicops-internals-arm-v6plus.h:268:8: error: instruction requires: arm-mode "strexd %0, %2, [%3]\n"` BUG=564059 ========== to ========== Disable thumb when building tcmalloc This fixes the arm/linux build which is currently broken because third_party/tc_malloc is not currently buildable with -mthumb: atomicops-internals-arm-v6plus.h:268:8: error: instruction requires: arm-mode "strexd %0, %2, [%3]\n"` BUG=564059 Committed: https://crrev.com/446886fca650e9239999df634ef6bc890b95680d Cr-Commit-Position: refs/heads/master@{#433052} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/446886fca650e9239999df634ef6bc890b95680d Cr-Commit-Position: refs/heads/master@{#433052}
Message was sent while issue was closed.
On 2016/11/18 03:22:27, commit-bot: I haz the power wrote: > Patchset 2 (id:??) landed as > https://crrev.com/446886fca650e9239999df634ef6bc890b95680d > Cr-Commit-Position: refs/heads/master@{#433052} I think this code is not working for ChromeOS +config("compiler_arm_thumb") { + if (current_cpu == "arm" && arm_use_thumb && is_posix && + !(is_mac || is_ios || is_nacl)) { + cflags = [ "-mthumb" ] + if (is_android && !is_clang) { + # Clang doesn't support this option. + cflags += [ "-mthumb-interwork" ] + } + } +} +
Message was sent while issue was closed.
On 2016/11/21 18:33:32, llozano1 wrote: > On 2016/11/18 03:22:27, commit-bot: I haz the power wrote: > > Patchset 2 (id:??) landed as > > https://crrev.com/446886fca650e9239999df634ef6bc890b95680d > > Cr-Commit-Position: refs/heads/master@{#433052} > > I think this code is not working for ChromeOS > > +config("compiler_arm_thumb") { > + if (current_cpu == "arm" && arm_use_thumb && is_posix && > + !(is_mac || is_ios || is_nacl)) { > + cflags = [ "-mthumb" ] > + if (is_android && !is_clang) { > + # Clang doesn't support this option. > + cflags += [ "-mthumb-interwork" ] > + } > + } > +} > + Are you sure? If this code wasn't working you wouldn't see -mthumb on the command line at all, right? Which means the whole of chrome will be compiled in arm mode, which means that tcmalloc clang issue wouldn't appear.. i think.
Message was sent while issue was closed.
On 2016/11/21 18:41:05, Sam Clegg wrote: > On 2016/11/21 18:33:32, llozano1 wrote: > > On 2016/11/18 03:22:27, commit-bot: I haz the power wrote: > > > Patchset 2 (id:??) landed as > > > https://crrev.com/446886fca650e9239999df634ef6bc890b95680d > > > Cr-Commit-Position: refs/heads/master@{#433052} > > > > I think this code is not working for ChromeOS > > > > +config("compiler_arm_thumb") { > > + if (current_cpu == "arm" && arm_use_thumb && is_posix && > > + !(is_mac || is_ios || is_nacl)) { > > + cflags = [ "-mthumb" ] > > + if (is_android && !is_clang) { > > + # Clang doesn't support this option. > > + cflags += [ "-mthumb-interwork" ] > > + } > > + } > > +} > > + > > Are you sure? If this code wasn't working you wouldn't see -mthumb on the > command line at all, right? Which means the whole of chrome will be compiled in > arm mode, which means that tcmalloc clang issue wouldn't appear.. i think. @llozano - what does "not working for ChromeOS" mean? You aren't seeing -mthumb? You are seeing -mthumb but don't want it?
Message was sent while issue was closed.
On 2016/11/21 18:54:21, Dirk Pranke wrote: > On 2016/11/21 18:41:05, Sam Clegg wrote: > > On 2016/11/21 18:33:32, llozano1 wrote: > > > On 2016/11/18 03:22:27, commit-bot: I haz the power wrote: > > > > Patchset 2 (id:??) landed as > > > > https://crrev.com/446886fca650e9239999df634ef6bc890b95680d > > > > Cr-Commit-Position: refs/heads/master@{#433052} > > > > > > I think this code is not working for ChromeOS > > > > > > +config("compiler_arm_thumb") { > > > + if (current_cpu == "arm" && arm_use_thumb && is_posix && > > > + !(is_mac || is_ios || is_nacl)) { > > > + cflags = [ "-mthumb" ] > > > + if (is_android && !is_clang) { > > > + # Clang doesn't support this option. > > > + cflags += [ "-mthumb-interwork" ] > > > + } > > > + } > > > +} > > > + > > > > Are you sure? If this code wasn't working you wouldn't see -mthumb on the > > command line at all, right? Which means the whole of chrome will be compiled > in > > arm mode, which means that tcmalloc clang issue wouldn't appear.. i think. > > @llozano - what does "not working for ChromeOS" mean? You aren't seeing -mthumb? > You are seeing -mthumb but don't want it? ok, sorry, my mistake. This patch is working for ChromeOS, ie: we dont see the -mthumb in the command line (even though I dont fully understand why its working). however, the problem (not being able to build tcmalloc) still exists because on ChromeOS the default compiler flag has been set to -mthumb. That is, everything is built with -mthumb by default. Since the bug is in Clang's integrated assembler (I have verified the code builds ok with -no-integrated-as), can you please provide a fix to pass -no-integrated-as for tcmalloc until the bug is fixed in the integrated assembler? The other solution suggested has been to add ".arm" directives to the __asm__ sections. It works, but I dont think that is the better fix. The bug is in the integrated assembler, so lets disable the integrated assembler.
Message was sent while issue was closed.
On 2016/11/21 20:22:46, llozano wrote: > On 2016/11/21 18:54:21, Dirk Pranke wrote: > > On 2016/11/21 18:41:05, Sam Clegg wrote: > > > On 2016/11/21 18:33:32, llozano1 wrote: > > > > On 2016/11/18 03:22:27, commit-bot: I haz the power wrote: > > > > > Patchset 2 (id:??) landed as > > > > > https://crrev.com/446886fca650e9239999df634ef6bc890b95680d > > > > > Cr-Commit-Position: refs/heads/master@{#433052} > > > > > > > > I think this code is not working for ChromeOS > > > > > > > > +config("compiler_arm_thumb") { > > > > + if (current_cpu == "arm" && arm_use_thumb && is_posix && > > > > + !(is_mac || is_ios || is_nacl)) { > > > > + cflags = [ "-mthumb" ] > > > > + if (is_android && !is_clang) { > > > > + # Clang doesn't support this option. > > > > + cflags += [ "-mthumb-interwork" ] > > > > + } > > > > + } > > > > +} > > > > + > > > > > > Are you sure? If this code wasn't working you wouldn't see -mthumb on the > > > command line at all, right? Which means the whole of chrome will be > compiled > > in > > > arm mode, which means that tcmalloc clang issue wouldn't appear.. i think. > > > > @llozano - what does "not working for ChromeOS" mean? You aren't seeing > -mthumb? > > You are seeing -mthumb but don't want it? > > ok, sorry, my mistake. > This patch is working for ChromeOS, ie: we dont see the -mthumb in the command > line (even though I dont fully understand why its working). > > however, the problem (not being able to build tcmalloc) still exists because on > ChromeOS the default compiler flag has been set to -mthumb. That is, everything > is built with -mthumb by default. How is the -mthumb flag being set exactly? I can't see anywhere else other than this config that would set it. The change to base/allocator/BUILD.gn in this CL should remove this flag for tcmalloc. Did you apply this entire patch? > > Since the bug is in Clang's integrated assembler (I have verified the code > builds ok with -no-integrated-as), can you please provide a fix to pass > -no-integrated-as for tcmalloc until the bug is fixed in the integrated > assembler? > > The other solution suggested has been to add ".arm" directives to the __asm__ > sections. It works, but I dont think that is the better fix. The bug is in the > integrated assembler, so lets disable the integrated assembler.
Message was sent while issue was closed.
On 2016/11/21 20:27:40, Sam Clegg wrote: > On 2016/11/21 20:22:46, llozano wrote: > > On 2016/11/21 18:54:21, Dirk Pranke wrote: > > > On 2016/11/21 18:41:05, Sam Clegg wrote: > > > > On 2016/11/21 18:33:32, llozano1 wrote: > > > > > On 2016/11/18 03:22:27, commit-bot: I haz the power wrote: > > > > > > Patchset 2 (id:??) landed as > > > > > > https://crrev.com/446886fca650e9239999df634ef6bc890b95680d > > > > > > Cr-Commit-Position: refs/heads/master@{#433052} > > > > > > > > > > I think this code is not working for ChromeOS > > > > > > > > > > +config("compiler_arm_thumb") { > > > > > + if (current_cpu == "arm" && arm_use_thumb && is_posix && > > > > > + !(is_mac || is_ios || is_nacl)) { > > > > > + cflags = [ "-mthumb" ] > > > > > + if (is_android && !is_clang) { > > > > > + # Clang doesn't support this option. > > > > > + cflags += [ "-mthumb-interwork" ] > > > > > + } > > > > > + } > > > > > +} > > > > > + > > > > > > > > Are you sure? If this code wasn't working you wouldn't see -mthumb on > the > > > > command line at all, right? Which means the whole of chrome will be > > compiled > > > in > > > > arm mode, which means that tcmalloc clang issue wouldn't appear.. i think. > > > > > > @llozano - what does "not working for ChromeOS" mean? You aren't seeing > > -mthumb? > > > You are seeing -mthumb but don't want it? > > > > ok, sorry, my mistake. > > This patch is working for ChromeOS, ie: we dont see the -mthumb in the command > > line (even though I dont fully understand why its working). > > > > however, the problem (not being able to build tcmalloc) still exists because > on > > ChromeOS the default compiler flag has been set to -mthumb. That is, > everything > > is built with -mthumb by default. > > How is the -mthumb flag being set exactly? I can't see anywhere else other than > this config that would set it. > ChromeOS has a compiler wrapper that sets certain flags. Mostly for security flags. It also sets -mthumb when building with clang. The reason this was done is because ChromeOS had set -mthumb as the default while building GCC (GCC has a configuration mode to set -mthumb as the default). So, we just copied the same behavior for clang but had to put the code in the wrapper because clang does not have a configuration mode for it. Code that cannot be build with -mthumb should specify -marm (or put the .arm directives as suggested somewhere else). But, IMO, discussion about -mthumb is immaterial. > The change to base/allocator/BUILD.gn in this CL should remove this flag for > tcmalloc. Did you apply this entire patch? > as I said, the patch provided here does meet his objective (removing -mthumb from the tcmalloc command line). I did not have to apply it because it was already committed. However, this is not the right fix. The code is correct thumb2 assembly code. It is assembled correctly by the GNU assembler. There is no reason to build tcmalloc without -mthumb. The bug is in the clang integrated assembler. That is what we need to disable. > > > > Since the bug is in Clang's integrated assembler (I have verified the code > > builds ok with -no-integrated-as), can you please provide a fix to pass > > -no-integrated-as for tcmalloc until the bug is fixed in the integrated > > assembler? > > > > The other solution suggested has been to add ".arm" directives to the __asm__ > > sections. It works, but I dont think that is the better fix. The bug is in the > > integrated assembler, so lets disable the integrated assembler.
Message was sent while issue was closed.
If this is a valid thumb instruction, can someone file an LLVM bug about their integrated assembler not accepting it? On Mon, Nov 21, 2016 at 5:38 PM, <llozano@google.com> wrote: > On 2016/11/21 20:27:40, Sam Clegg wrote: > > On 2016/11/21 20:22:46, llozano wrote: > > > On 2016/11/21 18:54:21, Dirk Pranke wrote: > > > > On 2016/11/21 18:41:05, Sam Clegg wrote: > > > > > On 2016/11/21 18:33:32, llozano1 wrote: > > > > > > On 2016/11/18 03:22:27, commit-bot: I haz the power wrote: > > > > > > > Patchset 2 (id:??) landed as > > > > > > > https://crrev.com/446886fca650e9239999df634ef6bc890b95680d > > > > > > > Cr-Commit-Position: refs/heads/master@{#433052} > > > > > > > > > > > > I think this code is not working for ChromeOS > > > > > > > > > > > > +config("compiler_arm_thumb") { > > > > > > + if (current_cpu == "arm" && arm_use_thumb && is_posix && > > > > > > + !(is_mac || is_ios || is_nacl)) { > > > > > > + cflags = [ "-mthumb" ] > > > > > > + if (is_android && !is_clang) { > > > > > > + # Clang doesn't support this option. > > > > > > + cflags += [ "-mthumb-interwork" ] > > > > > > + } > > > > > > + } > > > > > > +} > > > > > > + > > > > > > > > > > Are you sure? If this code wasn't working you wouldn't see -mthumb > on > > the > > > > > command line at all, right? Which means the whole of chrome will be > > > compiled > > > > in > > > > > arm mode, which means that tcmalloc clang issue wouldn't appear.. i > think. > > > > > > > > @llozano - what does "not working for ChromeOS" mean? You aren't > seeing > > > -mthumb? > > > > You are seeing -mthumb but don't want it? > > > > > > ok, sorry, my mistake. > > > This patch is working for ChromeOS, ie: we dont see the -mthumb in the > command > > > line (even though I dont fully understand why its working). > > > > > > however, the problem (not being able to build tcmalloc) still exists > because > > on > > > ChromeOS the default compiler flag has been set to -mthumb. That is, > > everything > > > is built with -mthumb by default. > > > > How is the -mthumb flag being set exactly? I can't see anywhere else > other > than > > this config that would set it. > > > > ChromeOS has a compiler wrapper that sets certain flags. Mostly for > security > flags. > It also sets -mthumb when building with clang. The reason this was done is > because ChromeOS had set -mthumb as the default while building GCC (GCC > has a > configuration mode to set -mthumb as the default). So, we just copied the > same > behavior for clang but had to put the code in the wrapper because clang > does not > have a configuration mode for it. > Code that cannot be build with -mthumb should specify -marm (or put the > .arm > directives as suggested somewhere else). > But, IMO, discussion about -mthumb is immaterial. > > > The change to base/allocator/BUILD.gn in this CL should remove this flag > for > > tcmalloc. Did you apply this entire patch? > > > > as I said, the patch provided here does meet his objective (removing > -mthumb > from the tcmalloc command line). I did not have to apply it because it was > already committed. > > However, this is not the right fix. The code is correct thumb2 assembly > code. It > is assembled correctly by the GNU assembler. > There is no reason to build tcmalloc without -mthumb. > > The bug is in the clang integrated assembler. That is what we need to > disable. > > > > > > > Since the bug is in Clang's integrated assembler (I have verified the > code > > > builds ok with -no-integrated-as), can you please provide a fix to pass > > > -no-integrated-as for tcmalloc until the bug is fixed in the integrated > > > assembler? > > > > > > The other solution suggested has been to add ".arm" directives to the > __asm__ > > > sections. It works, but I dont think that is the better fix. The bug > is in > the > > > integrated assembler, so lets disable the integrated assembler. > > > > https://codereview.chromium.org/2515503002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/11/21 22:46:02, Nico wrote: > If this is a valid thumb instruction, can someone file an LLVM bug about > their integrated assembler not accepting it? I already filed a bug, and added a TODO in the BUILD.gn: https://codereview.chromium.org/2514903002 In the interim I chose to disable thumb for this part of the code rather than build with -no-integrated-as. Using -no-integrated-as might work too (but watch out for adding a dependency on arm-linux-gnueabihf-as.. its been a while and I can't remember exactly how that works on the bots and with goma). > > On Mon, Nov 21, 2016 at 5:38 PM, <mailto:llozano@google.com> wrote: > > > On 2016/11/21 20:27:40, Sam Clegg wrote: > > > On 2016/11/21 20:22:46, llozano wrote: > > > > On 2016/11/21 18:54:21, Dirk Pranke wrote: > > > > > On 2016/11/21 18:41:05, Sam Clegg wrote: > > > > > > On 2016/11/21 18:33:32, llozano1 wrote: > > > > > > > On 2016/11/18 03:22:27, commit-bot: I haz the power wrote: > > > > > > > > Patchset 2 (id:??) landed as > > > > > > > > https://crrev.com/446886fca650e9239999df634ef6bc890b95680d > > > > > > > > Cr-Commit-Position: refs/heads/master@{#433052} > > > > > > > > > > > > > > I think this code is not working for ChromeOS > > > > > > > > > > > > > > +config("compiler_arm_thumb") { > > > > > > > + if (current_cpu == "arm" && arm_use_thumb && is_posix && > > > > > > > + !(is_mac || is_ios || is_nacl)) { > > > > > > > + cflags = [ "-mthumb" ] > > > > > > > + if (is_android && !is_clang) { > > > > > > > + # Clang doesn't support this option. > > > > > > > + cflags += [ "-mthumb-interwork" ] > > > > > > > + } > > > > > > > + } > > > > > > > +} > > > > > > > + > > > > > > > > > > > > Are you sure? If this code wasn't working you wouldn't see -mthumb > > on > > > the > > > > > > command line at all, right? Which means the whole of chrome will be > > > > compiled > > > > > in > > > > > > arm mode, which means that tcmalloc clang issue wouldn't appear.. i > > think. > > > > > > > > > > @llozano - what does "not working for ChromeOS" mean? You aren't > > seeing > > > > -mthumb? > > > > > You are seeing -mthumb but don't want it? > > > > > > > > ok, sorry, my mistake. > > > > This patch is working for ChromeOS, ie: we dont see the -mthumb in the > > command > > > > line (even though I dont fully understand why its working). > > > > > > > > however, the problem (not being able to build tcmalloc) still exists > > because > > > on > > > > ChromeOS the default compiler flag has been set to -mthumb. That is, > > > everything > > > > is built with -mthumb by default. > > > > > > How is the -mthumb flag being set exactly? I can't see anywhere else > > other > > than > > > this config that would set it. > > > > > > > ChromeOS has a compiler wrapper that sets certain flags. Mostly for > > security > > flags. > > It also sets -mthumb when building with clang. The reason this was done is > > because ChromeOS had set -mthumb as the default while building GCC (GCC > > has a > > configuration mode to set -mthumb as the default). So, we just copied the > > same > > behavior for clang but had to put the code in the wrapper because clang > > does not > > have a configuration mode for it. > > Code that cannot be build with -mthumb should specify -marm (or put the > > .arm > > directives as suggested somewhere else). > > But, IMO, discussion about -mthumb is immaterial. > > > > > The change to base/allocator/BUILD.gn in this CL should remove this flag > > for > > > tcmalloc. Did you apply this entire patch? > > > > > > > as I said, the patch provided here does meet his objective (removing > > -mthumb > > from the tcmalloc command line). I did not have to apply it because it was > > already committed. > > > > However, this is not the right fix. The code is correct thumb2 assembly > > code. It > > is assembled correctly by the GNU assembler. > > There is no reason to build tcmalloc without -mthumb. > > > > The bug is in the clang integrated assembler. That is what we need to > > disable. > > > > > > > > > > Since the bug is in Clang's integrated assembler (I have verified the > > code > > > > builds ok with -no-integrated-as), can you please provide a fix to pass > > > > -no-integrated-as for tcmalloc until the bug is fixed in the integrated > > > > assembler? > > > > > > > > The other solution suggested has been to add ".arm" directives to the > > __asm__ > > > > sections. It works, but I dont think that is the better fix. The bug > > is in > > the > > > > integrated assembler, so lets disable the integrated assembler. > > > > > > > > https://codereview.chromium.org/2515503002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
cool, thanks! On Mon, Nov 21, 2016 at 5:55 PM, <sbc@chromium.org> wrote: > On 2016/11/21 22:46:02, Nico wrote: > > If this is a valid thumb instruction, can someone file an LLVM bug about > > their integrated assembler not accepting it? > > I already filed a bug, and added a TODO in the BUILD.gn: > https://codereview.chromium.org/2514903002 > > In the interim I chose to disable thumb for this part of the code rather > than > build with -no-integrated-as. Using -no-integrated-as might work too (but > watch > out for adding a dependency on arm-linux-gnueabihf-as.. its been a while > and I > can't remember exactly how that works on the bots and with goma). > > > > > > > On Mon, Nov 21, 2016 at 5:38 PM, <mailto:llozano@google.com> wrote: > > > > > On 2016/11/21 20:27:40, Sam Clegg wrote: > > > > On 2016/11/21 20:22:46, llozano wrote: > > > > > On 2016/11/21 18:54:21, Dirk Pranke wrote: > > > > > > On 2016/11/21 18:41:05, Sam Clegg wrote: > > > > > > > On 2016/11/21 18:33:32, llozano1 wrote: > > > > > > > > On 2016/11/18 03:22:27, commit-bot: I haz the power wrote: > > > > > > > > > Patchset 2 (id:??) landed as > > > > > > > > > https://crrev.com/446886fca650e9239999df634ef6bc890b95680d > > > > > > > > > Cr-Commit-Position: refs/heads/master@{#433052} > > > > > > > > > > > > > > > > I think this code is not working for ChromeOS > > > > > > > > > > > > > > > > +config("compiler_arm_thumb") { > > > > > > > > + if (current_cpu == "arm" && arm_use_thumb && is_posix && > > > > > > > > + !(is_mac || is_ios || is_nacl)) { > > > > > > > > + cflags = [ "-mthumb" ] > > > > > > > > + if (is_android && !is_clang) { > > > > > > > > + # Clang doesn't support this option. > > > > > > > > + cflags += [ "-mthumb-interwork" ] > > > > > > > > + } > > > > > > > > + } > > > > > > > > +} > > > > > > > > + > > > > > > > > > > > > > > Are you sure? If this code wasn't working you wouldn't see > -mthumb > > > on > > > > the > > > > > > > command line at all, right? Which means the whole of chrome > will be > > > > > compiled > > > > > > in > > > > > > > arm mode, which means that tcmalloc clang issue wouldn't > appear.. i > > > think. > > > > > > > > > > > > @llozano - what does "not working for ChromeOS" mean? You aren't > > > seeing > > > > > -mthumb? > > > > > > You are seeing -mthumb but don't want it? > > > > > > > > > > ok, sorry, my mistake. > > > > > This patch is working for ChromeOS, ie: we dont see the -mthumb in > the > > > command > > > > > line (even though I dont fully understand why its working). > > > > > > > > > > however, the problem (not being able to build tcmalloc) still > exists > > > because > > > > on > > > > > ChromeOS the default compiler flag has been set to -mthumb. That > is, > > > > everything > > > > > is built with -mthumb by default. > > > > > > > > How is the -mthumb flag being set exactly? I can't see anywhere else > > > other > > > than > > > > this config that would set it. > > > > > > > > > > ChromeOS has a compiler wrapper that sets certain flags. Mostly for > > > security > > > flags. > > > It also sets -mthumb when building with clang. The reason this was > done is > > > because ChromeOS had set -mthumb as the default while building GCC (GCC > > > has a > > > configuration mode to set -mthumb as the default). So, we just copied > the > > > same > > > behavior for clang but had to put the code in the wrapper because clang > > > does not > > > have a configuration mode for it. > > > Code that cannot be build with -mthumb should specify -marm (or put the > > > .arm > > > directives as suggested somewhere else). > > > But, IMO, discussion about -mthumb is immaterial. > > > > > > > The change to base/allocator/BUILD.gn in this CL should remove this > flag > > > for > > > > tcmalloc. Did you apply this entire patch? > > > > > > > > > > as I said, the patch provided here does meet his objective (removing > > > -mthumb > > > from the tcmalloc command line). I did not have to apply it because it > was > > > already committed. > > > > > > However, this is not the right fix. The code is correct thumb2 assembly > > > code. It > > > is assembled correctly by the GNU assembler. > > > There is no reason to build tcmalloc without -mthumb. > > > > > > The bug is in the clang integrated assembler. That is what we need to > > > disable. > > > > > > > > > > > > > Since the bug is in Clang's integrated assembler (I have verified > the > > > code > > > > > builds ok with -no-integrated-as), can you please provide a fix to > pass > > > > > -no-integrated-as for tcmalloc until the bug is fixed in the > integrated > > > > > assembler? > > > > > > > > > > The other solution suggested has been to add ".arm" directives to > the > > > __asm__ > > > > > sections. It works, but I dont think that is the better fix. The > bug > > > is in > > > the > > > > > integrated assembler, so lets disable the integrated assembler. > > > > > > > > > > > > https://codereview.chromium.org/2515503002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/2515503002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/11/21 22:38:38, llozano1 wrote: > On 2016/11/21 20:27:40, Sam Clegg wrote: > > On 2016/11/21 20:22:46, llozano wrote: > > > On 2016/11/21 18:54:21, Dirk Pranke wrote: > > > > On 2016/11/21 18:41:05, Sam Clegg wrote: > > > > > On 2016/11/21 18:33:32, llozano1 wrote: > > > > > > On 2016/11/18 03:22:27, commit-bot: I haz the power wrote: > > > > > > > Patchset 2 (id:??) landed as > > > > > > > https://crrev.com/446886fca650e9239999df634ef6bc890b95680d > > > > > > > Cr-Commit-Position: refs/heads/master@{#433052} > > > > > > > > > > > > I think this code is not working for ChromeOS > > > > > > > > > > > > +config("compiler_arm_thumb") { > > > > > > + if (current_cpu == "arm" && arm_use_thumb && is_posix && > > > > > > + !(is_mac || is_ios || is_nacl)) { > > > > > > + cflags = [ "-mthumb" ] > > > > > > + if (is_android && !is_clang) { > > > > > > + # Clang doesn't support this option. > > > > > > + cflags += [ "-mthumb-interwork" ] > > > > > > + } > > > > > > + } > > > > > > +} > > > > > > + > > > > > > > > > > Are you sure? If this code wasn't working you wouldn't see -mthumb on > > the > > > > > command line at all, right? Which means the whole of chrome will be > > > compiled > > > > in > > > > > arm mode, which means that tcmalloc clang issue wouldn't appear.. i > think. > > > > > > > > @llozano - what does "not working for ChromeOS" mean? You aren't seeing > > > -mthumb? > > > > You are seeing -mthumb but don't want it? > > > > > > ok, sorry, my mistake. > > > This patch is working for ChromeOS, ie: we dont see the -mthumb in the > command > > > line (even though I dont fully understand why its working). > > > > > > however, the problem (not being able to build tcmalloc) still exists because > > on > > > ChromeOS the default compiler flag has been set to -mthumb. That is, > > everything > > > is built with -mthumb by default. > > > > How is the -mthumb flag being set exactly? I can't see anywhere else other > than > > this config that would set it. > > > > ChromeOS has a compiler wrapper that sets certain flags. Mostly for security > flags. > It also sets -mthumb when building with clang. The reason this was done is > because ChromeOS had set -mthumb as the default while building GCC (GCC has a > configuration mode to set -mthumb as the default). So, we just copied the same > behavior for clang but had to put the code in the wrapper because clang does not > have a configuration mode for it. > Code that cannot be build with -mthumb should specify -marm (or put the .arm > directives as suggested somewhere else). > But, IMO, discussion about -mthumb is immaterial. Ah... If your compiler wrapper is defaulting the -mthumb, that breaks that current gn expectation (that the default is arm). > > > The change to base/allocator/BUILD.gn in this CL should remove this flag for > > tcmalloc. Did you apply this entire patch? > > > > as I said, the patch provided here does meet his objective (removing -mthumb > from the tcmalloc command line). I did not have to apply it because it was > already committed. > > However, this is not the right fix. The code is correct thumb2 assembly code. It > is assembled correctly by the GNU assembler. > There is no reason to build tcmalloc without -mthumb. > > The bug is in the clang integrated assembler. That is what we need to disable. > > > > > > > Since the bug is in Clang's integrated assembler (I have verified the code > > > builds ok with -no-integrated-as), can you please provide a fix to pass > > > -no-integrated-as for tcmalloc until the bug is fixed in the integrated > > > assembler? > > > > > > The other solution suggested has been to add ".arm" directives to the > __asm__ > > > sections. It works, but I dont think that is the better fix. The bug is in > the > > > integrated assembler, so lets disable the integrated assembler.
Message was sent while issue was closed.
On 2016/11/21 22:59:38, Sam Clegg wrote: > On 2016/11/21 22:38:38, llozano1 wrote: > > On 2016/11/21 20:27:40, Sam Clegg wrote: > > > On 2016/11/21 20:22:46, llozano wrote: > > > > On 2016/11/21 18:54:21, Dirk Pranke wrote: > > > > > On 2016/11/21 18:41:05, Sam Clegg wrote: > > > > > > On 2016/11/21 18:33:32, llozano1 wrote: > > > > > > > On 2016/11/18 03:22:27, commit-bot: I haz the power wrote: > > > > > > > > Patchset 2 (id:??) landed as > > > > > > > > https://crrev.com/446886fca650e9239999df634ef6bc890b95680d > > > > > > > > Cr-Commit-Position: refs/heads/master@{#433052} > > > > > > > > > > > > > > I think this code is not working for ChromeOS > > > > > > > > > > > > > > +config("compiler_arm_thumb") { > > > > > > > + if (current_cpu == "arm" && arm_use_thumb && is_posix && > > > > > > > + !(is_mac || is_ios || is_nacl)) { > > > > > > > + cflags = [ "-mthumb" ] > > > > > > > + if (is_android && !is_clang) { > > > > > > > + # Clang doesn't support this option. > > > > > > > + cflags += [ "-mthumb-interwork" ] > > > > > > > + } > > > > > > > + } > > > > > > > +} > > > > > > > + > > > > > > > > > > > > Are you sure? If this code wasn't working you wouldn't see -mthumb > on > > > the > > > > > > command line at all, right? Which means the whole of chrome will be > > > > compiled > > > > > in > > > > > > arm mode, which means that tcmalloc clang issue wouldn't appear.. i > > think. > > > > > > > > > > @llozano - what does "not working for ChromeOS" mean? You aren't seeing > > > > -mthumb? > > > > > You are seeing -mthumb but don't want it? > > > > > > > > ok, sorry, my mistake. > > > > This patch is working for ChromeOS, ie: we dont see the -mthumb in the > > command > > > > line (even though I dont fully understand why its working). > > > > > > > > however, the problem (not being able to build tcmalloc) still exists > because > > > on > > > > ChromeOS the default compiler flag has been set to -mthumb. That is, > > > everything > > > > is built with -mthumb by default. > > > > > > How is the -mthumb flag being set exactly? I can't see anywhere else other > > than > > > this config that would set it. > > > > > > > ChromeOS has a compiler wrapper that sets certain flags. Mostly for security > > flags. > > It also sets -mthumb when building with clang. The reason this was done is > > because ChromeOS had set -mthumb as the default while building GCC (GCC has a > > configuration mode to set -mthumb as the default). So, we just copied the same > > behavior for clang but had to put the code in the wrapper because clang does > not > > have a configuration mode for it. > > Code that cannot be build with -mthumb should specify -marm (or put the .arm > > directives as suggested somewhere else). > > But, IMO, discussion about -mthumb is immaterial. > > Ah... If your compiler wrapper is defaulting the -mthumb, that breaks that > current gn expectation (that the default is arm). > I think you cannot assume a default. from GCC's options page https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html " -mthumb -marm Select between generating code that executes in ARM and Thumb states. The default for most configurations is to generate code that executes in ARM state, but the default can be changed by configuring GCC with the --with-mode=state configure option. You can also override the ARM and Thumb mode for each function by using the target("thumb") and target("arm") function attributes (see ARM Function Attributes) or pragmas (see Function Specific Option Pragmas) " ChromeOS has many examples of packages explicitly using one or the other. You could modify this CL to pass explicitly -mthumb or -marm.. and that should workaround the problem but I still think it is the wrong solution since what is broken is the integrated assembler not the assembly code written in the header. > > > > > The change to base/allocator/BUILD.gn in this CL should remove this flag for > > > tcmalloc. Did you apply this entire patch? > > > > > > > as I said, the patch provided here does meet his objective (removing -mthumb > > from the tcmalloc command line). I did not have to apply it because it was > > already committed. > > > > However, this is not the right fix. The code is correct thumb2 assembly code. > It > > is assembled correctly by the GNU assembler. > > There is no reason to build tcmalloc without -mthumb. > > > > The bug is in the clang integrated assembler. That is what we need to disable. > > > > > > > > > > > Since the bug is in Clang's integrated assembler (I have verified the code > > > > builds ok with -no-integrated-as), can you please provide a fix to pass > > > > -no-integrated-as for tcmalloc until the bug is fixed in the integrated > > > > assembler? > > > > > > > > The other solution suggested has been to add ".arm" directives to the > > __asm__ > > > > sections. It works, but I dont think that is the better fix. The bug is in > > the > > > > integrated assembler, so lets disable the integrated assembler.
Message was sent while issue was closed.
On 2016/11/22 00:36:47, llozano wrote: > I think you cannot assume a default. I'm not sure what you mean by this?
Message was sent while issue was closed.
On 2016/11/22 00:52:45, Dirk Pranke wrote: > On 2016/11/22 00:36:47, llozano wrote: > > I think you cannot assume a default. > > I'm not sure what you mean by this? Packages should specify one or the other if their code can only be built with one of them. Otherwise, dont specify anything and that means it can be build with any. That is what happens when I continue discussing what I should not continue discussing.. Please backtrack: why are you trying to disable -mthumb when the assembly code is perfectly legal -mthumb? mcgrathr also said it in https://bugs.chromium.org/p/chromium/issues/detail?id=564059 please reply to that. You guys seem to have been ignoring this point for a few iterations.
Message was sent while issue was closed.
On 2016/11/22 01:09:15, llozano wrote: > On 2016/11/22 00:52:45, Dirk Pranke wrote: > > On 2016/11/22 00:36:47, llozano wrote: > > > I think you cannot assume a default. > > > > I'm not sure what you mean by this? > > Packages should specify one or the other if their code can only be built with > one of them. Otherwise, dont specify anything and that means it can be build > with any. > > That is what happens when I continue discussing what I should not continue > discussing.. > > Please backtrack: > > why are you trying to disable -mthumb when the assembly code is perfectly legal > -mthumb? > > mcgrathr also said it in > https://bugs.chromium.org/p/chromium/issues/detail?id=564059 > > please reply to that. You guys seem to have been ignoring this point for a few > iterations. Sorry, I'm not trying to ignore that. I agree that it sounds like a better solution is to turn off integrated as, if that works. However, my concern is that we have yet another place where CrOS is attempting to set flags outside of the Chromium build and at cross-purposes. And so I was trying to understand if you're saying that generally we should never pass -mthumb, because it's not needed, or something else. My understanding is that we need to pass -mthumb if we want it, and so in order for CrOS to play nicely with us, you should not *also* be passing -mthumb, you should either not be passing anything, or setting the GN arg use_arm_thumb=false if you don't want it. Does that make sense? I'm no ARM compiler expert, so I don't know what the behavior is otherwise.
Message was sent while issue was closed.
On 2016/11/22 01:09:15, llozano wrote: > On 2016/11/22 00:52:45, Dirk Pranke wrote: > > On 2016/11/22 00:36:47, llozano wrote: > > > I think you cannot assume a default. > > > > I'm not sure what you mean by this? > > Packages should specify one or the other if their code can only be built with > one of them. Otherwise, dont specify anything and that means it can be build > with any. To me it makes sense if gn's configuration is in control of weather we build for thumb or not. If that requires always explicitly specifying either -mthumb or -marm we could do that I suppose, to avoid relying on a system default. I'm guessing you can't remove the -mthumb flag from your compiler wrapper because its being used to build more than just chrome? Is there some way to avoid using the wrapper when building chrome? > > That is what happens when I continue discussing what I should not continue > discussing.. > > Please backtrack: > > why are you trying to disable -mthumb when the assembly code is perfectly legal > -mthumb? > > mcgrathr also said it in > https://bugs.chromium.org/p/chromium/issues/detail?id=564059 > > please reply to that. You guys seem to have been ignoring this point for a few > iterations. See #27 for my rationale for disabling thumb in this module as a workaround for the clang bug. I did respond to Roland by updating the TODO and filing a clang bug. If you prefer the -no-integrated-as workaround we could go that route instead.
Message was sent while issue was closed.
On 2016/11/22 01:23:25, Sam Clegg wrote: > See #27 for my rationale for disabling thumb in this module as a workaround for > the clang bug. I did respond to Roland by updating the TODO and filing a clang > bug. > > If you prefer the -no-integrated-as workaround we could go that route instead. --no-integrated-as seems like a better route to me, at least based on what llozano's saying ...
Message was sent while issue was closed.
On 2016/11/22 01:18:19, Dirk Pranke wrote: > On 2016/11/22 01:09:15, llozano wrote: > > On 2016/11/22 00:52:45, Dirk Pranke wrote: > > > On 2016/11/22 00:36:47, llozano wrote: > > > > I think you cannot assume a default. > > > > > > I'm not sure what you mean by this? > > > > Packages should specify one or the other if their code can only be built with > > one of them. Otherwise, dont specify anything and that means it can be build > > with any. > > > > That is what happens when I continue discussing what I should not continue > > discussing.. > > > > Please backtrack: > > > > why are you trying to disable -mthumb when the assembly code is perfectly > legal > > -mthumb? > > > > mcgrathr also said it in > > https://bugs.chromium.org/p/chromium/issues/detail?id=564059 > > > > please reply to that. You guys seem to have been ignoring this point for a few > > iterations. > > Sorry, I'm not trying to ignore that. I agree that it sounds like a better > solution > is to turn off integrated as, if that works. > Thanks! I believe disabling the integrated assembler is the best solution but see my next reply. > However, my concern is that we have yet another place where CrOS is attempting > to set flags outside of the Chromium build and at cross-purposes. And so I was > trying to understand if you're saying that generally we should never pass > -mthumb, > because it's not needed, or something else. No, this option is weird. You set the default when you configure the compiler. In chromeos, we set the default to -mthumb because we want everything build with -mthumb (code size is very important). However, we will respect what GN wants but GN has to be explicit about it. If tcmalloc cannot be build with mthumb, then it needs to pass -marm. There is a lot of packages that already do this. See: https://cs.corp.google.com/search/?q=%5C-marm+package:%5Echromeos_public$&p=5... > My understanding is that we need to > pass -mthumb if we want it, and so in order for CrOS to play nicely with us, you > should not *also* be passing -mthumb, you should either not be passing anything, > or setting the GN arg use_arm_thumb=false if you don't want it. We just have a different default -mthumb.. SO, what I am saying is GN should be explicit and not assume a default. > > Does that make sense? I'm no ARM compiler expert, so I don't know what the > behavior is otherwise. It is a confusing option that can lead to several interpretations.
Message was sent while issue was closed.
On 2016/11/22 01:18:19, Dirk Pranke wrote: > On 2016/11/22 01:09:15, llozano wrote: > > On 2016/11/22 00:52:45, Dirk Pranke wrote: > > > On 2016/11/22 00:36:47, llozano wrote: > > > > I think you cannot assume a default. > > > > > > I'm not sure what you mean by this? > > > > Packages should specify one or the other if their code can only be built with > > one of them. Otherwise, dont specify anything and that means it can be build > > with any. > > > > That is what happens when I continue discussing what I should not continue > > discussing.. > > > > Please backtrack: > > > > why are you trying to disable -mthumb when the assembly code is perfectly > legal > > -mthumb? > > > > mcgrathr also said it in > > https://bugs.chromium.org/p/chromium/issues/detail?id=564059 > > > > please reply to that. You guys seem to have been ignoring this point for a few > > iterations. > > Sorry, I'm not trying to ignore that. I agree that it sounds like a better > solution > is to turn off integrated as, if that works. > Thanks! I believe disabling the integrated assembler is the best solution but see my next reply. > However, my concern is that we have yet another place where CrOS is attempting > to set flags outside of the Chromium build and at cross-purposes. And so I was > trying to understand if you're saying that generally we should never pass > -mthumb, > because it's not needed, or something else. No, this option is weird. You set the default when you configure the compiler. In chromeos, we set the default to -mthumb because we want everything build with -mthumb (code size is very important). However, we will respect what GN wants but GN has to be explicit about it. If tcmalloc cannot be build with mthumb, then it needs to pass -marm. There is a lot of packages that already do this. See: https://cs.corp.google.com/search/?q=%5C-marm+package:%5Echromeos_public$&p=5... > My understanding is that we need to > pass -mthumb if we want it, and so in order for CrOS to play nicely with us, you > should not *also* be passing -mthumb, you should either not be passing anything, > or setting the GN arg use_arm_thumb=false if you don't want it. We just have a different default -mthumb.. SO, what I am saying is GN should be explicit and not assume a default. > > Does that make sense? I'm no ARM compiler expert, so I don't know what the > behavior is otherwise. It is a confusing option that can lead to several interpretations.
Message was sent while issue was closed.
On 2016/11/22 01:23:25, Sam Clegg wrote: > On 2016/11/22 01:09:15, llozano wrote: > > On 2016/11/22 00:52:45, Dirk Pranke wrote: > > > On 2016/11/22 00:36:47, llozano wrote: > > > > I think you cannot assume a default. > > > > > > I'm not sure what you mean by this? > > > > Packages should specify one or the other if their code can only be built with > > one of them. Otherwise, dont specify anything and that means it can be build > > with any. > > To me it makes sense if gn's configuration is in control of weather we build for > thumb or not. If that requires always explicitly specifying either -mthumb or > -marm we could do that I suppose, to avoid relying on a system default. > yes, I think this can be solved by being explicit about it. As I said, it is not clear there is a system default. You set the default when you build the compiler. > I'm guessing you can't remove the -mthumb flag from your compiler wrapper > because its being used to build more than just chrome? Yes, the wrapper is used to build everything. But the wrapper is careful to set mthumb so that it can be overwritten. It is just a default. So, GN can ask for whatever it wants. > Is there some way to > avoid using the wrapper when building chrome? > No, wrapper is used to set security flags. > > > > That is what happens when I continue discussing what I should not continue > > discussing.. > > > > Please backtrack: > > > > why are you trying to disable -mthumb when the assembly code is perfectly > legal > > -mthumb? > > > > mcgrathr also said it in > > https://bugs.chromium.org/p/chromium/issues/detail?id=564059 > > > > please reply to that. You guys seem to have been ignoring this point for a few > > iterations. > > See #27 for my rationale for disabling thumb in this module as a workaround for > the clang bug. I did respond to Roland by updating the TODO and filing a clang > bug. I did not think this could cause issues with Goma. I really doubt goma would have removed the dependency on GNU assembler at this point. We use goma on ChromeOS and gnu assembler is used in most of the project (not CHrome browser). > > If you prefer the -no-integrated-as workaround we could go that route instead.
Message was sent while issue was closed.
On 2016/11/22 01:39:28, Dirk Pranke wrote: > On 2016/11/22 01:23:25, Sam Clegg wrote: > > See #27 for my rationale for disabling thumb in this module as a workaround > for > > the clang bug. I did respond to Roland by updating the TODO and filing a > clang > > bug. > > > > If you prefer the -no-integrated-as workaround we could go that route instead. > > --no-integrated-as seems like a better route to me, at least based on what > llozano's saying ... I will let you guys decide. I believe the cleanest solution is to disable integrated assembler because that is where the bug is (not in the assembly code) But since we only need a workaround until integrated assembler is fixed, then I understand your current CL is a faster solution with small side effects as long as we can make it work with ChromeOS. So, if you can explicitly pass -marm in this CL and we verify it works (it should) , then I am happy with it.
Message was sent while issue was closed.
that works for me.
Message was sent while issue was closed.
On 2016/11/22 02:19:55, Dirk Pranke wrote: > that works for me. I implemented to -no-integrated-assembler option: https://codereview.chromium.org/2518423002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
