Closed
Bug 654996
Opened 13 years ago
Closed 13 years ago
enable warnings as error for Mac
Categories
(Tamarin Graveyard :: Build Config, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: pnkfelix, Assigned: pnkfelix)
References
Details
Attachments
(2 files, 2 obsolete files)
4.16 KB,
patch
|
jsudduth
:
review+
lhansen
:
feedback+
|
Details | Diff | Splinter Review |
2.33 KB,
patch
|
jsudduth
:
review+
|
Details | Diff | Splinter Review |
We currently have warnings-as-errors turned on for Linux and Windows, but not Mac. This means engineers using Macs can overlook warnings in their builds that will cause the Linux and Windows builds to fail. It would be better for xplat and the Xcode project to turn on warnings-as-errors. This ticket is largely meant as a tracker for other problems that need to be fixed before warnings-as-errors can be turned on.
Assignee | ||
Comment 1•13 years ago
|
||
There is a warning that we cannot yet eliminate and that we cannot selectively turn off: ../core/Interpreter.cpp: In function ‘avmplus::Atom avmplus::interpBoxed(avmplus::MethodEnv*, int, avmplus::Atom*)’: ../core/Interpreter.cpp:744: warning: variable ‘pc’ might be clobbered by ‘longjmp’ or ‘vfork’ ../core/Interpreter.cpp:792: warning: variable ‘globalScope’ might be clobbered by ‘longjmp’ or ‘vfork’ Its not feasible (at least for me) to restructure the Interpreter to avoid this warning. And there are plenty of comments in the Interpreter.cpp source saying that the variable declarations should not be made volatile. Meanwhile, "-Wno-clobbered" does not appear to work -- yay gcc. :( So instead, I suggest turning on warnings-as-errors across the build, but selectively turn it off for Interpreter.cpp
Comment 2•13 years ago
|
||
You can't avoid this warning with gcc 4.0. And with 4.0 you can't selectively disable it (at least not on the command line). This is the main reason why we're running without errors as warnings on mac. I had hoped things would be better with gcc 4.2.
Assignee | ||
Comment 3•13 years ago
|
||
FYI: The current approach for selective enabling of -Werror dates from changeset 1703:6eae9388a11d http://hg.mozilla.org/tamarin-redux/rev/1703
Assignee | ||
Comment 4•13 years ago
|
||
(In reply to comment #2) > You can't avoid this warning with gcc 4.0. And with 4.0 you can't selectively > disable it (at least not on the command line). This is the main reason why > we're running without errors as warnings on mac. I had hoped things would be > better with gcc 4.2. See comment 1. This is what I'm suggesting: diff --git a/core/manifest.mk b/core/manifest.mk --- a/core/manifest.mk +++ b/core/manifest.mk @@ -144,8 +144,10 @@ avmplus_CXXSRCS := $(avmplus_CXXSRCS) \ # paths for code being generated outside build dir. # # 2. Use of '$(curdir)/AbcData.$(II_SUFFIX)' is also deliberate: # preprocessed file as target must be specified via same path that # is used in root manifest.mk. # # Further discussion at Bug 632086. $(curdir)/AbcData.$(II_SUFFIX): $(topsrcdir)/generated/builtin.cpp + +$(curdir)/Interpreter.$(OBJ_SUFFIX): avmplus_CXXFLAGS += -Wno-error
Comment 5•13 years ago
|
||
I guess. I never use the cross-platform build system on Mac (or Windows). Something that works with Xcode would really be required to call the problem solved.
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to comment #4) > > This is what I'm suggesting: Of course it would be better if the disabling of -Werror were isolated to Mac and did not apply to Linux or Windows builds. But that's more effort than I want to put into this at the moment; the *idea* is the important thing. :)
Assignee | ||
Comment 7•13 years ago
|
||
(In reply to comment #5) > I guess. I never use the cross-platform build system on Mac (or Windows). > Something that works with Xcode would really be required to call the problem > solved. Agreed; what I posted won't work for Xcode. I'll investigate whether an analogous change to that in comment 4 can be applied to Xcode. I won't post anything for review unless I can devise something that works the same in both contexts.
Assignee | ||
Comment 8•13 years ago
|
||
that was a lot easier than I expected.
Assignee | ||
Comment 9•13 years ago
|
||
(In reply to comment #8) > that was a lot easier than I expected. (but -- why am i not seeing the SelftextExec warning that prompted this investigation in the first place?)
Assignee | ||
Comment 10•13 years ago
|
||
(In reply to comment #9) > (In reply to comment #8) > > that was a lot easier than I expected. > > (but -- why am i not seeing the SelftextExec warning that prompted this > investigation in the first place?) Oh, because the checkbox for -Wunused-parameter is not checked. Making the various build configurations all consistent with one another is not in the scope of this bug.
Updated•13 years ago
|
Flags: flashplayer-bug-
Assignee | ||
Comment 11•13 years ago
|
||
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > that was a lot easier than I expected. > > > > (but -- why am i not seeing the SelftextExec warning that prompted this > > investigation in the first place?) > > Oh, because the checkbox for -Wunused-parameter is not checked. > > Making the various build configurations all consistent with one another is not > in the scope of this bug. No, that wasn't the answer. (I'm not going to spend too much more time on this, but there is definitely a mystery here...)
Assignee | ||
Comment 12•13 years ago
|
||
Discovery: the gcc invocation that Xcode uses is putting the -Wunused-parameter flag somewhere on the command line that is too early and it is getting disabled by a later parameter on the command line. (I discovered this by manually adding the flag to the end of the command line, after a fruitless investigation of whether precompiled headers were somehow related to the problem.) Anyway, its one of the following options that is causing unused parameter warning to be cancelled: -Wextra -Wno-invalid-offsetof -Wreorder -Wcast-align -Wdisabled-optimization -Winit-self -Winvalid-pch -Wpointer-arith -Wwrite-strings -Woverloaded-virtual -Wsign-promo -Wno-char-subscripts -Wstrict-aliasing=0 -Wstrict-null-sentinel \ Binary search time!
Assignee | ||
Comment 13•13 years ago
|
||
Oh jeez: if you put -Wunused-parameter *before* -Wextra, it gets disabled. It has to come after -Wextra. That seems totally wrong.
Assignee | ||
Comment 14•13 years ago
|
||
Even simpler! I keep forgetting about the xcconfig files (and Xcode isn't much help in reminding me about them).
Attachment #530304 -
Attachment is obsolete: true
Attachment #530339 -
Flags: review?(jsudduth)
Attachment #530339 -
Flags: feedback?(lhansen)
Assignee | ||
Comment 15•13 years ago
|
||
(Bug 655035 does not actually block this, it just is an off-shoot bit of work that I discovered in comment 9 through comment 12.)
Comment 16•13 years ago
|
||
Comment on attachment 530339 [details] [diff] [review] Xcode enable -Werror except for Interpreter.cpp Sweet.
Attachment #530339 -
Flags: feedback?(lhansen) → feedback+
Comment 17•13 years ago
|
||
Comment on attachment 530339 [details] [diff] [review] Xcode enable -Werror except for Interpreter.cpp +, but there's an extra "};" at the end of the exclusion line for Interpreter.cpp.
Attachment #530339 -
Flags: review?(jsudduth) → review+
Assignee | ||
Comment 18•13 years ago
|
||
(In reply to comment #17) > Comment on attachment 530339 [details] [diff] [review] [review] > Xcode enable -Werror except for Interpreter.cpp > > +, but there's an extra "};" at the end of the exclusion line for > Interpreter.cpp. I do not understand. Are you talking about the repeated character sequence "}; };" ? I think the first "};" is newly introduced to terminate the newly introduced "settings = { ..." entry
Assignee | ||
Comment 19•13 years ago
|
||
It still needs to be put through its paces in the sandbox system, but I think this patch captures the right logic.
Attachment #530381 -
Flags: review?(jsudduth)
Assignee | ||
Comment 20•13 years ago
|
||
Comment on attachment 530381 [details] [diff] [review] xplat enable -Werror on Mac, except for Interpreter.cpp on Mac. Not ready for prime time yet; buildbot shows that mips and android both dislike having -Werror turned on. (Android has the same problem Mac OS X has, so the same solution will probably apply there. MIPS ... MIPS I'm guessing I'll just have to continue to leave -Werror off for.)
Attachment #530381 -
Flags: review?(jsudduth)
Assignee | ||
Comment 21•13 years ago
|
||
Assignee: nobody → fklockii
Attachment #530381 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #530663 -
Flags: review?(jsudduth)
Assignee | ||
Comment 22•13 years ago
|
||
(In reply to comment #21) > Created attachment 530663 [details] [diff] [review] [review] > xplat enable -Werror (except for Interpreter.cpp) on Mac and Android (FYI this patch passes the buildbot sandbox test.)
Updated•13 years ago
|
Attachment #530663 -
Flags: review?(jsudduth) → review+
Comment 23•13 years ago
|
||
changeset: 6279:009f2e939c38 user: Felix S Klock II <fklockii@adobe.com> summary: Bug 654996: enable -Werror (except Interpreter.cpp) for Mac/Android, xcode+xplat (r=jsudduth). http://hg.mozilla.org/tamarin-redux/rev/009f2e939c38
Comment 24•13 years ago
|
||
changeset: 6280:8bc13bcc543c user: Felix S Klock II <fklockii@adobe.com> summary: Bug 654996: avm64 needed -Wno-error loving just like avm "avm32" (r=fklockii). http://hg.mozilla.org/tamarin-redux/rev/8bc13bcc543c
Assignee | ||
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•