399647 - OS/2 build break in nsprpub/pr/src/io/prlog.c

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a9pre) Gecko/2007101000 Minefield/3.0a9pre
Build Identifier: 

E:/usr/src/mozilla/nsprpub/pr/src/io/prlog.c: In function `PR_LogPrint':
E:/usr/src/mozilla/nsprpub/pr/src/io/prlog.c:495: error: assignment of read-only location
E:/usr/src/mozilla/nsprpub/pr/src/io/prlog.c:495: error: assignment of read-only location
E:/usr/src/mozilla/nsprpub/pr/src/io/prlog.c:497: warning: implicit declaration
of function `md_UnlockAndPostNotifies'
make.exe[6]: *** [prlog.o] Error 1


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Walter Meinl]

the offending line
http://mxr.mozilla.org/seamonkey/source/nsprpub/pr/src/io/prlog.c#495

Comment 2

[Walter Meinl]

Attachment: change of 1 and "\n" compiles

I have no clue if that's allowed, but it works with regard to compilation and a working browser

Comment 3

[David G King]

Confirming bug on WinXP with cygwin.

Also agree that this bug was introduced by fix for Bug #244478

Comment 4

[Wan-Teh Chang]

Hi.  Please try this:

 494         if (!nb || (line_long[nb-1] != '\n')) {
                 char eol[2];
                 eol[0] = '\n';
                 eol[1] = '\0';
 495             _PUT_LOG(logFile, eol, 1);
 496         }

Thanks!

Comment 5

[David G King]

The attached patch doesn't work for me. Different OS, etc, probably the cause.

However, the patch in Comment #4 allows code to compile.
A complete build is several hours away....

Comment 6

[Peter Weilbacher]

Yes, with the eol variable it works on OS/2. The shorter
   if (!nb || (line_long[nb-1] != '\n')) {
     char eol[] = "\n\0";
     _PUT_LOG(logFile, eol, 1);
   }
also works.

I still get a warning about the following line _PR_UNLOCK_LOG();
   M:/trunk/mozilla/nsprpub/pr/src/io/prlog.c:498: warning:
      implicit declaration of function `md_UnlockAndPostNotifies'
but that doesn't seem to be harmful -- and not easily fixed without starting to include .c files...

Comment 7

[Walter Meinl]

Comment on attachment 284691 [details] [diff] [review]
change of 1 and "\n" compiles

confirming also that the Wan-Teh's patch works. It took me a while to come back, since the browser crashes at certain pages but that seems not to be related to this bug (an older non-crashing build runs stable with the nspr dll's compiled using this patch)

Comment 8

[Wan-Teh Chang]

Peter, the initialization of an automatic (as opposed to static)
array is a new feature of C.  This is why I use assignments.

You can fix the undeclared function warning in two ways.

1. In _os2.h, declare md_UnlockAndPostNotifies before the
definition of _MD_UNLOCK:

  /* --- Lock stuff --- */
  #define _PR_LOCK                      _MD_LOCK
  #define _PR_UNLOCK                    _MD_UNLOCK
+ extern void md_UnlockAndPostNotifies(_MDLock *lock, PRThread *waitThred,
+                                      _MDCVar *waitCV);
  #ifdef USE_RAMSEM
  #define _MD_NEW_LOCK                  (_PR_MD_NEW_LOCK)

2. Define _MD_UNLOCK as a function:

In _os2.h, do this:

  #define _MD_UNLOCK                    (_PR_MD_UNLOCK)

Define the _PR_MD_UNLOCK function in os2cv.c.

Comment 9

[Peter Weilbacher]

Attachment: fix build and remove warning

OK, this now compiles without warnings but I haven't really tested it yet. (The USE_RAMSEM part is not interesting any more, I should actually update the patch in bug 330720 to remove it completely.)

Comment 10

[Wan-Teh Chang]

Comment on attachment 284786 [details] [diff] [review]
fix build and remove warning

r=wtc.

>+void _PR_MD_UNLOCK(_MDLock *lock)
>+{
>+    if (0 != (lock)->notified.length) {
>+        md_UnlockAndPostNotifies((lock), NULL, NULL);
>+    } else {
>+        DosReleaseMutexSem((lock)->mutex);
>+    }
>+}

When defined as a function, you can remove the parentheses
around the parameter 'lock'.

Comment 11

[Peter Weilbacher]

Attachment: [checked in] fix v2

Of course, this removes the brackets. Carrying over r+. (In the meantime I also tested PR logging, to confirm that after the build break fix it works again.)

Do I still need a sr for NSPR HEAD?

Comment 12

[Wan-Teh Chang]

Comment on attachment 284995 [details] [diff] [review]
[checked in] fix v2

Peter, please go ahead and check in this patch on the NSPR trunk.
Then, please create a new static tag NSPR_HEAD_yyyymmdd for mozilla/client.mk
and request approval.  (Mozilla trunk is pulling the NSPR tag specified in
mozilla/client.mk.)

Comment 13

[Peter Weilbacher]

Comment on attachment 284995 [details] [diff] [review]
[checked in] fix v2

Checked this into NSPR trunk.

Comment 14

[Peter Weilbacher]

Attachment: [checked in] Move NSPR tag in client.mk

OK, to activate this fix on trunk, we need to move the NSPR tag. The only change on NSPR trunk between these tags is the OS/2 change of attachment 284995 [details] [diff] [review].

Comment 15

[Peter Weilbacher]

Comment on attachment 285121 [details] [diff] [review]
[checked in] Move NSPR tag in client.mk

Checked this into trunk.

Comment 16

[Peter Weilbacher]

With that this bug should be fixed.