Closed Bug 1580260 Opened 5 years ago Closed 5 years ago

TestMustReturnFromCaller.cpp broken with clang trunk

Categories

(Developer Infrastructure :: Source Code Analysis, defect, P1)

defect

Tracking

(firefox74 fixed)

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: away, Assigned: andi)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

error: 'error' diagnostics seen but not expected:
TestMustReturnFromCaller.cpp Line 62: You must immediately return after calling this function
TestMustReturnFromCaller.cpp Line 128: You must immediately return after calling this function

Bisection points to LLVM r370247. I can't say whether this is a bug upstream or a change that our static analysis needs to adapt to. (Note that in that Phabricator, there is mention of a followup change 370406; it doesn't help for this issue.)

There is a bug in the CFG generator since we don't have the ReturnStmt into the CFGBlock but instead we have an IntegerLiteral followed by the ReturnStmt which is wrong, since IntegerLiteral should have parent the ReturnStmt.

The priority flag is not set for this bug.
:Sylvestre, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(sledru)

Andi; what do you think the priority should be?

Flags: needinfo?(sledru) → needinfo?(bpostelnicu)

If I remove that test file from the build, then it fails at https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/dom/bindings/BindingUtils.h#2480

[task 2019-09-30T22:58:09.281Z] 22:58:09    ERROR -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:2480:5: error: You must immediately return after calling this function
[task 2019-09-30T22:58:09.281Z] 22:58:09     INFO -      aRv.Throw(NS_ERROR_FAILURE);
[task 2019-09-30T22:58:09.281Z] 22:58:09     INFO -      ^

I guess the destructor for the nsCOMPtr is in the way?

Anyway, since blocks building the primary codebase (not "just tests") I think it should be relatively high priority.

Indeed i concur to what :dmajor is saying, this should be P1, even though it's related with the trunk version of clang.

Flags: needinfo?(bpostelnicu)

The priority flag is not set for this bug.
:Sylvestre, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(sledru)

Actually, it is a P2 because clang 10 isn't going to be released in before one or two firefox releases.

Flags: needinfo?(sledru)
Priority: -- → P2

(In reply to Sylvestre Ledru [:Sylvestre] from comment #7)

Actually, it is a P2 because clang 10 isn't going to be released in before one or two firefox releases.

Hello from three months in the future. :-) This is preventing from testing the clang 10 release candidates, can we reprioritize?

Flags: needinfo?(sledru)

Sure! Time flies!

Flags: needinfo?(sledru)
Priority: P2 → P1

Andi, if I'm reading your comment 1 correctly, this sounds like a bug in the clang source, is that right? If so, would you be able to file a bug against LLVM and explain the details there? We would need to do this fairly soon as we are running out of runway for the 10.0 release.

Flags: needinfo?(bpostelnicu)

Yes, it's a bug in the CFG generator but maybe in the meantime we can have a workaround for this in order to have this unblocked. I'm assigning this to myself to work on it.
Also will open a bug on llvm side and upstream a potential fix.

Assignee: nobody → bpostelnicu
Flags: needinfo?(bpostelnicu)

Starting with Clang-10, and in particular with https://reviews.llvm.org/D66404,
the CFG has been reflown in order to reduce discrepancies in destructor calls with
the actual codegen. This has also impacted the order of CFGStmt for ReturnStmt,
whereas now the AST children of the ReturnStmt have precedence other their parent
node.
The purpose of this patch is to fix this and add new supported cases for the Expr
that is generated from ReturnStmt.

Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b476d3ea3fc9
rework the logic for `MustReturnFromCaller` checker. r=froydnj
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: