Closed Bug 1326289 Opened 3 years ago Closed 3 years ago

Assertion failure when building clang-plugin against debug clang

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Nika, Assigned: Nika)

Details

Attachments

(1 file)

bool llvm::APInt::ult(const llvm::APInt&) const: Assertion
> `BitWidth == RHS.BitWidth && "Bit widths must be same for comparison"

Probably related to this line:

    if (Type->getSize().ule(Literal->getValue())) {
MozReview-Commit-ID: JNPc4CqEgPd
Attachment #8828140 - Flags: review?(ehsan)
Comment on attachment 8828140 [details] [diff] [review]
Fix BitWidth assertion failure in debug clang-plugin

Review of attachment 8828140 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet, thanks a lot!

Do these fix all of the assertions we hit?  Do you know what the overhead of having assertions on is?  I wonder if it's worth turning them on in our infrastructure?
Attachment #8828140 - Flags: review?(ehsan) → review+
Yes, I successfully ran an assertions enabled build on linux64 with clang 5.0 (trunk).

With regard to the cost of assertions is - I ran the build twice - once with a local release build running the clang plugin and once with a release build with assertions. The computer _was_ in use when both of these runs were done so it's not a perfect (or even "ok") comparison by any means.

With assertions disabled, a build of clang against my local tree took 21:46, while with assertions enabled it took 27:06 - so a pretty substantial hit. 

I would also be interested in potentially building clang-tidy with assertions enabled, and running that on try, as that could avoid a lot of the overhead from assertions during codegen which the clang-plugin would run into, but still makes the majority of the same checks. What do you think?
Flags: needinfo?(ehsan)
That would make the clang-tidy binaries that developers will use (post bug 1328454) slower, which isn't great...  I suppose we'll have to test this manually every once in a while then.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari from comment #4)
> That would make the clang-tidy binaries that developers will use (post bug
> 1328454) slower, which isn't great...  I suppose we'll have to test this
> manually every once in a while then.

Potentially we could build both? As in a debug one which is used on infra, and a release one which is downloaded by mach static-analysis?
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31b797ff2742
Fix BitWidth assertion failure in debug clang-plugin, r=ehsan
https://hg.mozilla.org/mozilla-central/rev/31b797ff2742
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.