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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: wuno, Assigned: mozilla)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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.
Posted patch change of 1 and "\n" compiles (obsolete) — Splinter Review
I have no clue if that's allowed, but it works with regard to compilation and a working browser
Confirming bug on WinXP with cygwin.

Also agree that this bug was introduced by fix for Bug #244478
Status: UNCONFIRMED → NEW
Ever confirmed: true
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!
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....
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 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)
Attachment #284691 - Attachment is obsolete: true
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.
Posted patch fix build and remove warning (obsolete) — Splinter Review
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 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'.
Attachment #284786 - Flags: review+
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?
Assignee: wtc → mozilla
Attachment #284786 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #284995 - Flags: review+
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 on attachment 284995 [details] [diff] [review]
[checked in] fix v2

Checked this into NSPR trunk.
Attachment #284995 - Attachment description: fix v2 → [checked in] fix v2
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].
Attachment #285121 - Flags: review?(benjamin)
Attachment #285121 - Flags: approval1.9?
Attachment #285121 - Flags: review?(benjamin)
Attachment #285121 - Flags: review+
Attachment #285121 - Flags: approval1.9?
Attachment #285121 - Flags: approval1.9+
Comment on attachment 285121 [details] [diff] [review]
[checked in] Move NSPR tag in client.mk

Checked this into trunk.
Attachment #285121 - Attachment description: Move NSPR tag in client.mk → [checked in] Move NSPR tag in client.mk
With that this bug should be fixed.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.