272624 - Build bustage on trunk of Firefox for OS/2

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Andy Willis (abwillis)]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8a6) Gecko/20041127
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.8a6) Gecko/20041127

Building Firefox from Trunk on OS/2 fails in logger.cpp and loghlp.cpp in
modules\plugin\tools\tester\common
This did not fail day before yesterday and hasn't been changed according to
lxr.mozilla.org since April.  Assuming it is related to checkin from bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=272189
which may have changed what is being built.
Including patch.

Reproducible: Always
Steps to Reproduce:
1.  Pull source from trunk for firefox.
2.  Attempt to build firefox on OS/2.
3.

Actual Results:  
Build failure.
(Affects OS/2 and maybe MingW.)

Expected Results:  
Build completes with no failures.

Comment 1

[Andy Willis (abwillis)]

Attachment: Corrects const char* to char* conversion error loghlp.cpp

Comment 2

[Andy Willis (abwillis)]

Attachment: Corrects const void* to void* conversion error logger.cpp

Comment 3

[Andy Willis (abwillis)]

Setting trunk on Version.

Comment 4

[Andy Willis (abwillis)]

Adding Mike and Peter to see if there is a better way to do this.

Comment 5

[Mike Kaply [:mkaply]]

I don't think there is. 

Comment 6

[Peter Weilbacher]

I think you can fill less of the fields in the bug entry form if you  
post enough error messages from the compile failure. I currently have
little time to build Firefox, so I don't know what the error could be.
Too bad, that we don't have a FF tinderbox for OS/2! What you correct
in your patches should normally only be reported as warnings not
errors that result in build failure. Bug 272189 is unlikely to be the
cause as it only changes what was normallly used as build
configuration.

(Btw, if you add patches, it makes more sense to use
   cvs diff -u filename(s) > file.diff
or perhaps the same with -u8. That gives you a diff in the standard   
format for all files, and you only need to attach once.)

Comment 7

[Mike Kaply [:mkaply]]

Are you using a later GCC? I get warnings for these but they don't stop my build....

Comment 8

[Andy Willis (abwillis)]

Yes, I am using 3.3.5 and not 3.2.2.  Just tried with 3.2.2 and it worked.  Last
time I saw these type errors I tested both and got the error on both so I didn't
realize there might be a difference this time.

Comment 9

[Andy Willis (abwillis)]

For the record:
E:/cvs/work/mozilla/modules/plugin/tools/tester/common/logger.cpp:291: error: in
valid
   conversion from `const void*' to `void*'
E:/cvs/work/mozilla/modules/plugin/tools/tester/common/logger.cpp: In member
   function `void CLogger::dumpLogToTarget()':
E:/cvs/work/mozilla/modules/plugin/tools/tester/common/logger.cpp:317: warning:
unused
   variable `int iLength'
make.exe[1]: *** [logger.o] Error 1
make.exe[1]: Leaving directory `E:/cvs/work/mozilla/ffobj/modules/plugin/tools/t
ester/common'
make.exe: *** [all] Error 2
I don't know why it suddenly started being a problem if it wasn't the bug
specified as the builds I said worked were with 3.3.5 as well.

Comment 10

[Mike Kaply [:mkaply]]

I brought some of these up with bird previously and he needs to fix them in the
compiler. The new GCC shouldn't be producing more errors than it did before.

Comment 11

[Mike Kaply [:mkaply]]

Knut,

Here are some more errors that only show up with the new GCC.

Comment 12

[knut st. osmundsen]

Arg. I just ignored this as some kind of weird annoyance when I encountered it
last week. But somethings seems messed up and for some really weird reasons - my
attempt on a simple testcase for it didn't work out. I'll take a closer look
later tonight.

Kind Regards,
 knut

Comment 13

[knut st. osmundsen]

I've managed to reduce these two problems to:

unsigned NPN_Write(void* buffer);
void clearTarget(void)
{
    NPN_Write("\n");
}

char * makeLogItemStruct(char *dw3)
{
    char* str = dw3 ? dw3 : "";
    return str;
}

Both the litterals will cause g++ to say:
error: invalid conversion from `const char*' to `char*'

I've verified that the above code will cause errors with the follwoing GCC versions:
  3.2.2 (OS/2)
  3.3.3 (mingw32)
  3.3.3 (OS/2)
  3.3.4 (Linux x86)
  3.3.5 (OS/2)
  3.4.2 (FreeBSD amd64)
  3.4.2 (FreeBSD x86)

OTOH, Visual C++ 7.0, Visual Age for C++ 3.08, Visual Age for C++ 3.6.5 and
Watcom C/C++ v11.0c don't say anything when subjected to the code.

Since so many GCC versions are broken/annoyingly-strict I don't see a big
argument whether this should be patched.


BTW. Some questions naturally rises from this issue. Are the OS/2 guys the only
ones actually compiling the tests regularly? Or do we just happen to be using
odd GCC version?  What GCC versions are used on other platforms?

Kind Regards,
 knut

Comment 14

[knut st. osmundsen]

FYI. On 3.2.2 the -pedantic option will convert the errors to a warning, that's
why I get errors when building it with no options (g++ -c moz-test-char.cpp).

Comment 15

[Bradley Baetz (:bbaetz)]

The warnig is correct, because if you tried to modify "" the behaviour is
undefined - it'll crash on unix and I beveli that msvc will put " into a
readonly area too.

Not 100% sure if it should be an error, mind you.

Comment 16

[Andy Willis (abwillis)]

In a roundabout way the bug I referenced was the cause.  I synced my .mozconfig
files and didn't put --disable-tests back in on my first run.  After fixing the
error and finishing the build I found all the test stuff and put it back in but
hadn't linked the 2 in my mind so normally even the OS/2 guys aren't building
the tests I don't think.  

Comment 17

[Peter Weilbacher]

(In reply to comment #13)
> I've verified that the above code will cause errors with the follwoing GCC
versions:
>   3.2.2 (OS/2)
>   3.3.3 (mingw32)
>   3.3.3 (OS/2)
>   3.3.4 (Linux x86)
>   3.3.5 (OS/2)
[...]
> Since so many GCC versions are broken/annoyingly-strict I don't see a big
> argument whether this should be patched.

As you noticed later yourself, gcc is called with several arguments for
compiling loglph.cpp, on my build these are

g++ -o loghlp.o -c -DOSTYPE=\"OS22\" -DOSARCH=\"OS2\"
-Im:/moztrunk/mozilla/modules/plugin/tools/tes

ter/common/../include  -I../../../../../dist/include/java
-I../../../../../dist/include/plugin -I../

../../../../dist/include/plugin -I../../../../../dist/include
-Im:/moztrunk/obj_noMN/dist/include/ns

pr    -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align
-Woverloaded-virtual

 -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic
-Zomf -pipe  -DNDEBUG

 -DTRIMMED -O2 -s   -DMOZILLA_CLIENT -include ../../../../../mozilla-config.h
-Uunix -U__unix -U__un

ix__ -Wp,-MD,.deps/loghlp.pp
m:/moztrunk/mozilla/modules/plugin/tools/tester/common/loghlp.cpp

And there I do see a difference between GCC 3.2.2 and GCC 3.3.5 beta on OS/2,
where the latter gives an error

loghlp.cpp:517: error: invalid conversion from `const char*' to `char*'

while the former just gives a warning for the same code. Unless this reflects a
difference in the GCC development (which I doubt becaue many people build
Mozilla without --disable-tests and with GCC 3.3.x and 3.4.x) this should be
changed in the OS/2 version.

Btw, I do build the tests from time to time (so does the tinderbox on the
Seamonkey-Ports page) but as this just gives a warning with the "standard" GCC
3.2.2 it was never noticed...

Comment 18

[knut st. osmundsen]

Which tinderbox are you talking about, the OS/2 one? The OS/2 one is of little
interest now. The question is if anyone else is using GCC 3.3.1+ and building tests.

And, of course, I did test with all the flags, and found that only the -pedantic
one + 3.2.2 made any difference, as I've already reported. Btw. the -pedantic
error => warning problem apparently was fixed in GCC 3.3.1 and GCC 3.4.0:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=10032

Any chance of that a linux developer/buildguy could verify that the tests are
broken on GCC 3.3.1+? There must be someone who tries to build the tests once in
a while...

Kind Regards,
 knut

Comment 19

[Peter Weilbacher]

I was talking about the OS/2 tinderbox, but the same is true for the other
tinderboxes, e.g. "brad" on
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey uses gcc 3.3.5 on
Linux and its .mozconfig does not seem to include --disable-tests. (I would
guess that all tinderboxes do build the tests because they need the tests to
check that the compile completed successfully.) Additionally, the box "balsa" on
the SeaMonkey-Ports page uses gcc 3.4.

Wait, the logs of these builds never contain anything about plugin/tools/tester.
Ah, that dir ever only gets used on OS/2, modules/plugin/Makefile.in contains:

 76 ifeq ($(OS_ARCH),OS2)
 77 DIRS            += samples/default/os2
 78 ifdef ENABLE_TESTS
 79 DIRS            += tools/tester/common tools/tester/os2
 80 endif
 81 endif

Comment 20

[Mike Kaply [:mkaply]]

I didn't think about that.

You're right, tester is a Windows OS/2 thing. I'll get this patch in.

Comment 21

[Peter Weilbacher]

Attachment: Fix all gcc335 errors + a few warnings

Mike, AFAICS you haven't checked anything in yet. I created this patch from
Andy's two patches and added fixes for three more warnings that also appear
with gcc322 when compiling modules/plugin/tools/tester/common/. I still get 

m:/trunk/mozilla/modules/plugin/tools/tester/include/log.h: In
   destructor `LogArgumentStruct::~LogArgumentStruct()':
m:/trunk/mozilla/modules/plugin/tools/tester/include/log.h:58: warning:
deleting
   `void*' is undefined

everytime but this would require more changes througout (although I this
warning seems to make sense). While I hope that the extras do not break
anything on Windows (if it is really still used there), feel free to just use
Andy's patches and ignore the extra stuff...

Comment 22

[Mike Kaply [:mkaply]]

Comment on attachment 170294 [details] [diff] [review]
Fix all gcc335 errors + a few warnings

>       // strlen not protected from 0!
>-      char* str = dw3 ? (LPSTR)dw3 : "";
>+      char* str = dw3 ? (LPSTR)dw3 : (char*)"";
>       plis->arg3.iLength = strlen(str) + 1;

For perfection sake, that LPSTR cast should probably be a char* cast

otherwise, r=mkaply

Comment 23

[Peter Weilbacher]

Yes, you are right. Do I need someone's sr for this (it seems previous checkins
were not reviewed at all)? Otherwise can you make that small change when
checking in or do you want me to upload a perfected patch?

Comment 24

[Mike Kaply [:mkaply]]

I'll fix it when I check in. tomorrow. really.

Comment 25

[Mike Kaply [:mkaply]]

Fix checked in.