Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2762233003:
[sql] Disable a clang intrinsic in SQLite 3.17.0. (Closed)
|
Created:
3 years, 9 months ago by Scott Hess - ex-Googler Modified:
3 years, 9 months ago CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[sql] Disable a clang intrinsic in SQLite 3.17.0.
SQLite implements sqlite3MulInt64() using __builtin_mul_overflow() for
appropriate versions of GCC or clang. But clang appears to provide this
function in a non-standard library which our Android clang does not provide. It
fails with an undefined symbol __mulodi4.
[It is possible that this is specific to Chromium's packaged clang, but
https://bugs.llvm.org//show_bug.cgi?id=28629 ]
BUG=701524, 701518
Review-Url: https://codereview.chromium.org/2762233003
Cr-Commit-Position: refs/heads/master@{#458457}
Committed: https://chromium.googlesource.com/chromium/src/+/9e39ce92107b239f0043d55eef91e155b1471382
Patch Set 1 #
Messages
Total messages: 25 (13 generated)
Description was changed from ========== [sql] Disable a clang intrinsic in SQLite 3.17.0. SQLite implements sqlite3MulInt64() using __builtin_mul_overflow() for appropriate versions of GCC or clang. But clang appears to provide this function in a non-standard library which our Android clang does not provide. It fails with an undefined symbol __mulodi4. [It is possible that this is specific to Chromium's packaged clang, but https://bugs.llvm.org//show_bug.cgi?id=28629 ] BUG=701518 TBR=michaeln@chromium.org ========== to ========== [sql] Disable a clang intrinsic in SQLite 3.17.0. SQLite implements sqlite3MulInt64() using __builtin_mul_overflow() for appropriate versions of GCC or clang. But clang appears to provide this function in a non-standard library which our Android clang does not provide. It fails with an undefined symbol __mulodi4. [It is possible that this is specific to Chromium's packaged clang, but https://bugs.llvm.org//show_bug.cgi?id=28629 ] BUG=701518 ==========
shess@chromium.org changed reviewers: - michaeln@chromium.org
shess@chromium.org changed reviewers: + stevenjb@chromium.org
WDYT? This basically reverts this function to the code we were using in the previous version of SQLite we used. Due to limitations in our code-review system, I'm running it through trybots over at: https://codereview.chromium.org/2767473003 Of course, that didn't catch this. It did catch variants of this, though, so hopefully it will cover the space well enough.
The CQ bit was checked by shess@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...
On 2017/03/21 16:14:41, Scott Hess wrote: > WDYT? This basically reverts this function to the code we were using in the > previous version of SQLite we used. > > Due to limitations in our code-review system, I'm running it through trybots > over at: > https://codereview.chromium.org/2767473003 > Of course, that didn't catch this. It did catch variants of this, though, so > hopefully it will cover the space well enough. Actually, let me give it a try on this CL - I'm still thinking in terms of the complete import, which definitely breaks the trybots nine ways, but maybe this would actually work because the patch is much more focussed.
On 2017/03/21 16:14:41, Scott Hess wrote: > WDYT? This basically reverts this function to the code we were using in the > previous version of SQLite we used. > > Due to limitations in our code-review system, I'm running it through trybots > over at: > https://codereview.chromium.org/2767473003 > Of course, that didn't catch this. It did catch variants of this, though, so > hopefully it will cover the space well enough. In theory the failure we are seeing in the PFQ should be caught on this builder: https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy... Otherwise I don't know enough about the specific problem to relaly review this, but if we think it will fix the PFQ, lgtm :) +llozano@
stevenjb@chromium.org changed reviewers: + llozano@chromium.org
+llozano@
The CQ bit was unchecked by shess@chromium.org
The CQ bit was checked by shess@chromium.org
The CQ bit was unchecked by shess@chromium.org
On 2017/03/21 16:25:02, stevenjb wrote: > On 2017/03/21 16:14:41, Scott Hess wrote: > > WDYT? This basically reverts this function to the code we were using in the > > previous version of SQLite we used. > > > > Due to limitations in our code-review system, I'm running it through trybots > > over at: > > https://codereview.chromium.org/2767473003 > > Of course, that didn't catch this. It did catch variants of this, though, so > > hopefully it will cover the space well enough. > > In theory the failure we are seeing in the PFQ should be caught on this builder: > https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy... OK, I'll add it to the list. > Otherwise I don't know enough about the specific problem to relaly review this, > but if we think it will fix the PFQ, lgtm :) Per the more-specific bug (I'll add that to the description), I'm not sure anyone knows enough about the specific problem to review this. I'm happy enough to just hammer on it until it stops hurting.
Description was changed from ========== [sql] Disable a clang intrinsic in SQLite 3.17.0. SQLite implements sqlite3MulInt64() using __builtin_mul_overflow() for appropriate versions of GCC or clang. But clang appears to provide this function in a non-standard library which our Android clang does not provide. It fails with an undefined symbol __mulodi4. [It is possible that this is specific to Chromium's packaged clang, but https://bugs.llvm.org//show_bug.cgi?id=28629 ] BUG=701518 ========== to ========== [sql] Disable a clang intrinsic in SQLite 3.17.0. SQLite implements sqlite3MulInt64() using __builtin_mul_overflow() for appropriate versions of GCC or clang. But clang appears to provide this function in a non-standard library which our Android clang does not provide. It fails with an undefined symbol __mulodi4. [It is possible that this is specific to Chromium's packaged clang, but https://bugs.llvm.org//show_bug.cgi?id=28629 ] BUG=701524,701518 ==========
On 2017/03/21 16:29:45, Scott Hess wrote: > On 2017/03/21 16:25:02, stevenjb wrote: > > On 2017/03/21 16:14:41, Scott Hess wrote: > > > WDYT? This basically reverts this function to the code we were using in the > > > previous version of SQLite we used. > > > > > > Due to limitations in our code-review system, I'm running it through trybots > > > over at: > > > https://codereview.chromium.org/2767473003 > > > Of course, that didn't catch this. It did catch variants of this, though, > so > > > hopefully it will cover the space well enough. > > > > In theory the failure we are seeing in the PFQ should be caught on this > builder: > > > https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy... > > OK, I'll add it to the list. > > > Otherwise I don't know enough about the specific problem to relaly review > this, > > but if we think it will fix the PFQ, lgtm :) > > Per the more-specific bug (I'll add that to the description), I'm not sure > anyone knows enough about the specific problem to review this. I'm happy enough > to just hammer on it until it stops hurting. How about we hammer in this nail, and if it doesn't fix it then maybe we take a step back and re-assess? :)
On 2017/03/21 16:32:57, stevenjb wrote: > On 2017/03/21 16:29:45, Scott Hess wrote: > > On 2017/03/21 16:25:02, stevenjb wrote: > > > On 2017/03/21 16:14:41, Scott Hess wrote: > > > > WDYT? This basically reverts this function to the code we were using in > the > > > > previous version of SQLite we used. > > > > > > > > Due to limitations in our code-review system, I'm running it through > trybots > > > > over at: > > > > https://codereview.chromium.org/2767473003 > > > > Of course, that didn't catch this. It did catch variants of this, though, > > so > > > > hopefully it will cover the space well enough. > > > > > > In theory the failure we are seeing in the PFQ should be caught on this > > builder: > > > > > > https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy... > > > > OK, I'll add it to the list. > > > > > Otherwise I don't know enough about the specific problem to relaly review > > this, > > > but if we think it will fix the PFQ, lgtm :) > > > > Per the more-specific bug (I'll add that to the description), I'm not sure > > anyone knows enough about the specific problem to review this. I'm happy > enough > > to just hammer on it until it stops hurting. > > How about we hammer in this nail, and if it doesn't fix it then maybe we take a > step back and re-assess? :) Hmm, it doesn't look like the simple chrome builders are catching this, otherwise it would be breaking on the main tree: https://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20daisy%... I'm not sure why, maybe it only affects official builds?
On 2017/03/21 16:43:05, stevenjb wrote: > On 2017/03/21 16:32:57, stevenjb wrote: > > On 2017/03/21 16:29:45, Scott Hess wrote: > > > On 2017/03/21 16:25:02, stevenjb wrote: > > > > On 2017/03/21 16:14:41, Scott Hess wrote: > > > > > WDYT? This basically reverts this function to the code we were using in > > the > > > > > previous version of SQLite we used. > > > > > > > > > > Due to limitations in our code-review system, I'm running it through > > trybots > > > > > over at: > > > > > https://codereview.chromium.org/2767473003 > > > > > Of course, that didn't catch this. It did catch variants of this, > though, > > > so > > > > > hopefully it will cover the space well enough. > > > > > > > > In theory the failure we are seeing in the PFQ should be caught on this > > > builder: > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy... > > > > > > OK, I'll add it to the list. > > > > > > > Otherwise I don't know enough about the specific problem to relaly review > > > this, > > > > but if we think it will fix the PFQ, lgtm :) > > > > > > Per the more-specific bug (I'll add that to the description), I'm not sure > > > anyone knows enough about the specific problem to review this. I'm happy > > enough > > > to just hammer on it until it stops hurting. > > > > How about we hammer in this nail, and if it doesn't fix it then maybe we take > a > > step back and re-assess? :) > > Hmm, it doesn't look like the simple chrome builders are catching this, > otherwise it would be breaking on the main tree: > https://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20daisy%... > > I'm not sure why, maybe it only affects official builds? Yeah, this is the crux of the problem, it's only biting in the long tail.
On 2017/03/21 16:48:25, Scott Hess wrote: > On 2017/03/21 16:43:05, stevenjb wrote: > > On 2017/03/21 16:32:57, stevenjb wrote: > > > On 2017/03/21 16:29:45, Scott Hess wrote: > > > > On 2017/03/21 16:25:02, stevenjb wrote: > > > > > On 2017/03/21 16:14:41, Scott Hess wrote: > > > > > > WDYT? This basically reverts this function to the code we were using > in > > > the > > > > > > previous version of SQLite we used. > > > > > > > > > > > > Due to limitations in our code-review system, I'm running it through > > > trybots > > > > > > over at: > > > > > > https://codereview.chromium.org/2767473003 > > > > > > Of course, that didn't catch this. It did catch variants of this, > > though, > > > > so > > > > > > hopefully it will cover the space well enough. > > > > > > > > > > In theory the failure we are seeing in the PFQ should be caught on this > > > > builder: > > > > > > > > > > > > > > > https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy... > > > > > > > > OK, I'll add it to the list. > > > > > > > > > Otherwise I don't know enough about the specific problem to relaly > review > > > > this, > > > > > but if we think it will fix the PFQ, lgtm :) > > > > > > > > Per the more-specific bug (I'll add that to the description), I'm not sure > > > > anyone knows enough about the specific problem to review this. I'm happy > > > enough > > > > to just hammer on it until it stops hurting. > > > > > > How about we hammer in this nail, and if it doesn't fix it then maybe we > take > > a > > > step back and re-assess? :) > > > > Hmm, it doesn't look like the simple chrome builders are catching this, > > otherwise it would be breaking on the main tree: > > > https://build.chromium.org/p/chromium.chromiumos/builders/ChromiumOS%20daisy%... > > > > I'm not sure why, maybe it only affects official builds? > > Yeah, this is the crux of the problem, it's only biting in the long tail. actually - I guess that implies that adding trybots isn't helpful. It's passed the main compiles, including one which was breaking earlier (prior to your issues), so I'm just going to take your LGTM and run with it.
The CQ bit was checked by shess@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": 1, "attempt_start_ts": 1490115019850000, "parent_rev":
"99854454417e7c9ad57d2d2d441ef17cc481ba2d", "commit_rev":
"9e39ce92107b239f0043d55eef91e155b1471382"}
Message was sent while issue was closed.
Description was changed from ========== [sql] Disable a clang intrinsic in SQLite 3.17.0. SQLite implements sqlite3MulInt64() using __builtin_mul_overflow() for appropriate versions of GCC or clang. But clang appears to provide this function in a non-standard library which our Android clang does not provide. It fails with an undefined symbol __mulodi4. [It is possible that this is specific to Chromium's packaged clang, but https://bugs.llvm.org//show_bug.cgi?id=28629 ] BUG=701524,701518 ========== to ========== [sql] Disable a clang intrinsic in SQLite 3.17.0. SQLite implements sqlite3MulInt64() using __builtin_mul_overflow() for appropriate versions of GCC or clang. But clang appears to provide this function in a non-standard library which our Android clang does not provide. It fails with an undefined symbol __mulodi4. [It is possible that this is specific to Chromium's packaged clang, but https://bugs.llvm.org//show_bug.cgi?id=28629 ] BUG=701524,701518 Review-Url: https://codereview.chromium.org/2762233003 Cr-Commit-Position: refs/heads/master@{#458457} Committed: https://chromium.googlesource.com/chromium/src/+/9e39ce92107b239f0043d55eef91... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/9e39ce92107b239f0043d55eef91...
Message was sent while issue was closed.
On 2017/03/21 17:35:43, commit-bot: I haz the power wrote: > Committed patchset #1 (id:1) as > https://chromium.googlesource.com/chromium/src/+/9e39ce92107b239f0043d55eef91... looks good to me... I don't know if this has been affecting Chrome OS. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
