Closed Bug 654996 Opened 13 years ago Closed 13 years ago

enable warnings as error for Mac

Categories

(Tamarin Graveyard :: Build Config, defect)

All
macOS
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: pnkfelix, Assigned: pnkfelix)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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
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.
FYI: The current approach for selective enabling of -Werror dates from 
changeset 1703:6eae9388a11d

  http://hg.mozilla.org/tamarin-redux/rev/1703
(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
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.
(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.  :)
(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.
that was a lot easier than I expected.
(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?)
(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.
Flags: flashplayer-bug-
(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...)
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!
Oh jeez: if you put -Wunused-parameter *before* -Wextra, it gets disabled.  It has to come after -Wextra.  That seems totally wrong.
Depends on: 655035
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)
(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.)
No longer depends on: 655035
See Also: → 655035
Comment on attachment 530339 [details] [diff] [review]
Xcode enable -Werror except for Interpreter.cpp

Sweet.
Attachment #530339 - Flags: feedback?(lhansen) → feedback+
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+
(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
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)
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: nobody → fklockii
Attachment #530381 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #530663 - Flags: review?(jsudduth)
(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.)
Attachment #530663 - Flags: review?(jsudduth) → review+
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
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
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.

Attachment

General

Created:
Updated:
Size: