Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2932043004:
clang/arm: Use integrated assembler for boringssl for arm_version 6. (Closed)
Descriptionclang/arm: Use integrated assembler for boringssl for arm_version 6.
An arm_version 6 file assumes that __clang__ means that clang's assembler is
being used, while it only really identifies clang is used as preprocessor.
Luckily, it looks like only the armv7 assembly code in boringssl is incompatible
with clang's built-in assembler.
BUG=732066, 124610
TBR=davidben
Review-Url: https://codereview.chromium.org/2932043004
Cr-Commit-Position: refs/heads/master@{#478524}
Committed: https://chromium.googlesource.com/chromium/src/+/a4fb1dcc42b4f733129e9c3dfba2add43c25d740
Patch Set 1 #Patch Set 2 : import #Messages
Total messages: 17 (11 generated)
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/v2/patch-status/codereview.chromium.or...
thakis@chromium.org changed reviewers: + davidben@chromium.org
This is just a workaround in the hope that I can keep my switch in a bit longer. The Right Fix is to keep reporting problems with clang's built-in assembler to upstream and then eventually rely on the fixes and have no silly workarounds in boringssl's asm. (As you say, due to Xcode, that'll take a while, but it'll happen eventually.)
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/v2/patch-status/codereview.chromium.or...
Description was changed from ========== clang/arm: Use integrated assembler for boringssl for arm_version 6. An arm_version 6 file assumes that __clang__ means that clang's assembler is being used, while it only really identifies clang is used as preprocessor. BUG=732066 ========== to ========== clang/arm: Use integrated assembler for boringssl for arm_version 6. An arm_version 6 file assumes that __clang__ means that clang's assembler is being used, while it only really identifies clang is used as preprocessor. Luckily, it looks like only the armv7 assembly code in boringssl is incompatible with clang's built-in assembler. BUG=732066,124610 ==========
Description was changed from ========== clang/arm: Use integrated assembler for boringssl for arm_version 6. An arm_version 6 file assumes that __clang__ means that clang's assembler is being used, while it only really identifies clang is used as preprocessor. Luckily, it looks like only the armv7 assembly code in boringssl is incompatible with clang's built-in assembler. BUG=732066,124610 ========== to ========== clang/arm: Use integrated assembler for boringssl for arm_version 6. An arm_version 6 file assumes that __clang__ means that clang's assembler is being used, while it only really identifies clang is used as preprocessor. Luckily, it looks like only the armv7 assembly code in boringssl is incompatible with clang's built-in assembler. BUG=732066,124610 TBR=davidben ==========
The CQ bit was unchecked by thakis@chromium.org
The CQ bit was checked by thakis@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497137724754560,
"parent_rev": "2ad97e65a9dd2ecb87342532c22d713f1841c621", "commit_rev":
"a4fb1dcc42b4f733129e9c3dfba2add43c25d740"}
Message was sent while issue was closed.
Description was changed from ========== clang/arm: Use integrated assembler for boringssl for arm_version 6. An arm_version 6 file assumes that __clang__ means that clang's assembler is being used, while it only really identifies clang is used as preprocessor. Luckily, it looks like only the armv7 assembly code in boringssl is incompatible with clang's built-in assembler. BUG=732066,124610 TBR=davidben ========== to ========== clang/arm: Use integrated assembler for boringssl for arm_version 6. An arm_version 6 file assumes that __clang__ means that clang's assembler is being used, while it only really identifies clang is used as preprocessor. Luckily, it looks like only the armv7 assembly code in boringssl is incompatible with clang's built-in assembler. BUG=732066,124610 TBR=davidben Review-Url: https://codereview.chromium.org/2932043004 Cr-Commit-Position: refs/heads/master@{#478524} Committed: https://chromium.googlesource.com/chromium/src/+/a4fb1dcc42b4f733129e9c3dfba2... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a4fb1dcc42b4f733129e9c3dfba2...
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
On 2017/06/10 22:50:25, Nico (vacation Jun 3-11) wrote: > This is just a workaround in the hope that I can keep my switch in a bit longer. > > The Right Fix is to keep reporting problems with clang's built-in assembler to > upstream and then eventually rely on the fixes and have no silly workarounds in > boringssl's asm. (As you say, due to Xcode, that'll take a while, but it'll > happen eventually.) In the meantime, does clang seriously not expose a preprocessor hint as to whether its using its own assembler?
Message was sent while issue was closed.
On 2017/06/11 04:38:50, davidben wrote: > On 2017/06/10 22:50:25, Nico (vacation Jun 3-11) wrote: > > This is just a workaround in the hope that I can keep my switch in a bit > longer. > > > > The Right Fix is to keep reporting problems with clang's built-in assembler to > > upstream and then eventually rely on the fixes and have no silly workarounds > in > > boringssl's asm. (As you say, due to Xcode, that'll take a while, but it'll > > happen eventually.) > > In the meantime, does clang seriously not expose a preprocessor hint as to > whether its using its own assembler? Actually, yeah, this CL cannot stay as-is very long. This is far too fragile and our team is stretched thin as it is without this kind of constraint. I'll see about generalizing the __APPLE__ workaround to all of __clang__. Waiting for clang to fix its assembler isn't an option due to how this code is maintained. |
