Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1484883002:
clang/arm: Push -no-integrated-as into the four targets that need it. (Closed)
Descriptionclang/arm: Push -no-integrated-as into the four targets that need it.
clang's integrated assembler does not support old-style ARM assembly,
but the assembly itself generates is always modern so it's ok to
use the integrated assembler for all .cc compilations. Three projects
have old and crufty .S ARM asm files, so push -no-integrated-as into these
three projects. We can then clean those up asynchronously, while this
change makes sure no other projects with old assembly make it into the
tree.
Depends on:
https://codereview.webrtc.org/1484103002/
https://codereview.webrtc.org/1486143002/
https://codereview.chromium.org/1485143002/
https://chromium-review.googlesource.com/#/c/315110/2
BUG=124610
Committed: https://crrev.com/7faa96991e5f362d1d1e92666ebedf870bad9140
Cr-Commit-Position: refs/heads/master@{#362584}
Patch Set 1 #
Total comments: 6
Patch Set 2 : linux arm #Patch Set 3 : typo #Patch Set 4 : gn #
Messages
Total messages: 29 (13 generated)
Description was changed from ========== clang/arm: Push -no-integrated-as into the three targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generated is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty ARM assembly, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. BUG= ========== to ========== clang/arm: Push -no-integrated-as into the three targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generated is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty ARM assembly, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. BUG=124610 ==========
Description was changed from ========== clang/arm: Push -no-integrated-as into the three targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generated is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty ARM assembly, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. BUG=124610 ========== to ========== clang/arm: Push -no-integrated-as into the three targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generated is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty ARM assembly, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on https://codereview.webrtc.org/1484103002/ BUG=124610 ==========
Argh no, this didn't land, https://codereview.webrtc.org/1484103002/ did :-/
Description was changed from ========== clang/arm: Push -no-integrated-as into the three targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generated is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty ARM assembly, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on https://codereview.webrtc.org/1484103002/ BUG=124610 ========== to ========== clang/arm: Push -no-integrated-as into the three targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generates is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty .S ARM asm files, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on https://codereview.webrtc.org/1484103002/ BUG=124610 ==========
The CQ bit was checked by thakis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1484883002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1484883002/1
thakis@chromium.org changed reviewers: + davidben@chromium.org, hans@chromium.org, johannkoenig@chromium.org
I accidentally emailed this out to nobody earlier today, so rietveld isn't including the change description. I'm pasting it manually below. johannkoenig: libvpx_new davidben: boringssl hans: rest clang/arm: Push -no-integrated-as into the three targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generates is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty .S ARM asm files, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on https://codereview.webrtc.org/1484103002/ BUG=124610
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, very nice https://codereview.chromium.org/1484883002/diff/1/build/config/android/BUILD.gn File build/config/android/BUILD.gn (right): https://codereview.chromium.org/1484883002/diff/1/build/config/android/BUILD.... build/config/android/BUILD.gn:36: if (current_cpu == "mipsel") { Is the MIPS build gn only?
sbc reminded me about the non-android arm build, which requires a bit more work (not too much). I'll try to do that too while I'm at it. https://codereview.chromium.org/1484883002/diff/1/build/config/android/BUILD.gn File build/config/android/BUILD.gn (right): https://codereview.chromium.org/1484883002/diff/1/build/config/android/BUILD.... build/config/android/BUILD.gn:36: if (current_cpu == "mipsel") { On 2015/12/01 15:18:24, hans wrote: > Is the MIPS build gn only? No, gyp has something like this somewhere too.
johannkoenig@google.com changed reviewers: + johannkoenig@google.com
libvpx LGTM - one typo in another file https://codereview.chromium.org/1484883002/diff/1/build/config/android/BUILD.gn File build/config/android/BUILD.gn (right): https://codereview.chromium.org/1484883002/diff/1/build/config/android/BUILD.... build/config/android/BUILD.gn:38: # TODO(gordanac) Enable integrated-as.com/124610). Typo in the comment https://codereview.chromium.org/1484883002/diff/1/third_party/libvpx_new/libv... File third_party/libvpx_new/libvpx.gyp (right): https://codereview.chromium.org/1484883002/diff/1/third_party/libvpx_new/libv... third_party/libvpx_new/libvpx.gyp:53: # TODO(hans) Enable integrated-as (crbug.com/124610). We can generate clang-specific assembly. The issue isn't old-and-crufty vs new-and-fancy, it's just GAS (gnu assembler syntax) vs clang which are *almost the same it's so close but not quite*. libvpx assembly is written in UAL which is for the ARM assembler. It was probably also called ADS at some point because our scripts are "ads2gas.pl" In ads2gas.gypi, we use the "clang" scripts (ads2gas_apple.pl because that was when we first needed it). BUILD.gn doesn't offer to run the apple version of the script. That script takes a -chromium argument because apple and chromium clang are *almost the same it's so close but not quite*
thanks! https://codereview.chromium.org/1484883002/diff/1/third_party/libvpx_new/libv... File third_party/libvpx_new/libvpx.gyp (right): https://codereview.chromium.org/1484883002/diff/1/third_party/libvpx_new/libv... third_party/libvpx_new/libvpx.gyp:53: # TODO(hans) Enable integrated-as (crbug.com/124610). On 2015/12/01 15:26:18, Johann wrote: > We can generate clang-specific assembly. The issue isn't old-and-crufty vs > new-and-fancy, it's just GAS (gnu assembler syntax) vs clang which are *almost > the same it's so close but not quite*. clang's intentions are to support UAL from what I understand. If it doesn't get something in UAL right then we can fix clang. I do intend to work on making the assembly work in both clang and gcc at some point, but I'd like to first convert most of the build (i.e. this CL). > libvpx assembly is written in UAL which > is for the ARM assembler. It was probably also called ADS at some point because > our scripts are "ads2gas.pl" > > In ads2gas.gypi, we use the "clang" scripts (ads2gas_apple.pl because that was > when we first needed it). BUILD.gn doesn't offer to run the apple version of the > script. That script takes a -chromium argument because apple and chromium clang > are *almost the same it's so close but not quite* If there are specific disagreements, please let me know about them :-)
On 2015/12/01 15:32:28, Nico wrote: > thanks! Sure, and this was just intended as FYI > https://codereview.chromium.org/1484883002/diff/1/third_party/libvpx_new/libv... > File third_party/libvpx_new/libvpx.gyp (right): > > https://codereview.chromium.org/1484883002/diff/1/third_party/libvpx_new/libv... > third_party/libvpx_new/libvpx.gyp:53: # TODO(hans) Enable integrated-as > (crbug.com/124610). > On 2015/12/01 15:26:18, Johann wrote: > > We can generate clang-specific assembly. The issue isn't old-and-crufty vs > > new-and-fancy, it's just GAS (gnu assembler syntax) vs clang which are *almost > > the same it's so close but not quite*. > > clang's intentions are to support UAL from what I understand. If it doesn't get > something in UAL right then we can fix clang. Huh, I've never tried UAL in clang. The trick there would be to, if clang, pipe it straight through 'cat' or something instead of the script. > I do intend to work on making the assembly work in both clang and gcc at some > point, but I'd like to first convert most of the build (i.e. this CL). For sure, just giving background. > > libvpx assembly is written in UAL which > > is for the ARM assembler. It was probably also called ADS at some point > because > > our scripts are "ads2gas.pl" > > > > In ads2gas.gypi, we use the "clang" scripts (ads2gas_apple.pl because that was > > when we first needed it). BUILD.gn doesn't offer to run the apple version of > the > > script. That script takes a -chromium argument because apple and chromium > clang > > are *almost the same it's so close but not quite* > > If there are specific disagreements, please let me know about them :-) https://chromium.googlesource.com/webm/libvpx/+/master/build/make/ads2gas_app... # Clang used by Chromium differs slightly from clang in XCode in what it # will accept in the assembly. if ($chromium) { s/qsubaddx/qsax/i; s/qaddsubx/qasx/i; s/ldrneb/ldrbne/i; s/ldrneh/ldrhne/i; s/(vqshrun\.s16 .*, \#)0$/${1}8/i; # http://llvm.org/bugs/show_bug.cgi?id=16022 s/\.include/#include/; }
Description was changed from ========== clang/arm: Push -no-integrated-as into the three targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generates is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty .S ARM asm files, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on https://codereview.webrtc.org/1484103002/ BUG=124610 ========== to ========== clang/arm: Push -no-integrated-as into the three targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generates is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty .S ARM asm files, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on: https://codereview.webrtc.org/1484103002/ https://codereview.webrtc.org/1486143002/ https://codereview.chromium.org/1485143002/ https://chromium-review.googlesource.com/#/c/315110/2 BUG=124610 ==========
ok, with patch set 2 and the longer "depends on" list, non-android linux/arm now builds for me too. https://codereview.chromium.org/1484883002/diff/1/build/config/android/BUILD.gn File build/config/android/BUILD.gn (right): https://codereview.chromium.org/1484883002/diff/1/build/config/android/BUILD.... build/config/android/BUILD.gn:38: # TODO(gordanac) Enable integrated-as.com/124610). On 2015/12/01 15:26:18, Johann wrote: > Typo in the comment Done.
third_party/boringssl lgtm Which assembly files are problematic on the BoringSSL side? Is it the perlasm ones, the couple of non-perlasm ones, or both?
I haven't checked yet. (But once this is in and you're curious, you can just remove the -fno-integrated-as flags from boringssl's build config files and do a local clang android build to see)
The CQ bit was checked by thakis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hans@chromium.org, johannkoenig@google.com Link to the patchset: https://codereview.chromium.org/1484883002/#ps60001 (title: "gn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1484883002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1484883002/60001
Description was changed from ========== clang/arm: Push -no-integrated-as into the three targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generates is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty .S ARM asm files, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on: https://codereview.webrtc.org/1484103002/ https://codereview.webrtc.org/1486143002/ https://codereview.chromium.org/1485143002/ https://chromium-review.googlesource.com/#/c/315110/2 BUG=124610 ========== to ========== clang/arm: Push -no-integrated-as into the four targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generates is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty .S ARM asm files, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on: https://codereview.webrtc.org/1484103002/ https://codereview.webrtc.org/1486143002/ https://codereview.chromium.org/1485143002/ https://chromium-review.googlesource.com/#/c/315110/2 BUG=124610 ==========
Message was sent while issue was closed.
Description was changed from ========== clang/arm: Push -no-integrated-as into the four targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generates is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty .S ARM asm files, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on: https://codereview.webrtc.org/1484103002/ https://codereview.webrtc.org/1486143002/ https://codereview.chromium.org/1485143002/ https://chromium-review.googlesource.com/#/c/315110/2 BUG=124610 ========== to ========== clang/arm: Push -no-integrated-as into the four targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generates is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty .S ARM asm files, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on: https://codereview.webrtc.org/1484103002/ https://codereview.webrtc.org/1486143002/ https://codereview.chromium.org/1485143002/ https://chromium-review.googlesource.com/#/c/315110/2 BUG=124610 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== clang/arm: Push -no-integrated-as into the four targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generates is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty .S ARM asm files, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on: https://codereview.webrtc.org/1484103002/ https://codereview.webrtc.org/1486143002/ https://codereview.chromium.org/1485143002/ https://chromium-review.googlesource.com/#/c/315110/2 BUG=124610 ========== to ========== clang/arm: Push -no-integrated-as into the four targets that need it. clang's integrated assembler does not support old-style ARM assembly, but the assembly itself generates is always modern so it's ok to use the integrated assembler for all .cc compilations. Three projects have old and crufty .S ARM asm files, so push -no-integrated-as into these three projects. We can then clean those up asynchronously, while this change makes sure no other projects with old assembly make it into the tree. Depends on: https://codereview.webrtc.org/1484103002/ https://codereview.webrtc.org/1486143002/ https://codereview.chromium.org/1485143002/ https://chromium-review.googlesource.com/#/c/315110/2 BUG=124610 Committed: https://crrev.com/7faa96991e5f362d1d1e92666ebedf870bad9140 Cr-Commit-Position: refs/heads/master@{#362584} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7faa96991e5f362d1d1e92666ebedf870bad9140 Cr-Commit-Position: refs/heads/master@{#362584} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
