Closed Bug 328989 Opened 18 years ago Closed 18 years ago

[BeOS] Build broken by patch to bug 296159

Categories

(Core Graveyard :: Plug-ins, defect)

x86
BeOS
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: doug, Assigned: alfred.peng)

Details

Attachments

(4 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20060225 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (BeOS; U; BeOS BePC; en-US; rv:1.9a1) Gecko/20060225 Firefox/1.6a1

BeOS building breaks in mozilla/modules/plugin/base/src/nsJSNPRuntime.cpp, apparently after fix to bug 296159 landed.

Reproducible: Always

Steps to Reproduce:
1.build firefox after 2006-02-28 under BeOS
2.wait for bustage
3.

Actual Results:  
build breaks

Expected Results:  
completed build
Component: General → Plug-ins
Product: Firefox → Core
Version: unspecified → Trunk
as far I understand, using PR* types everywhere in Mozilla is preferable.
wondering  where from that uint32_t passed in.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch Patchv1 (obsolete) — Splinter Review
Since PR* types are cross platform, we need to use it in the NPRuntime plugin.

Besides the enumeration function, I also change the invoke and invokedefault functions. I don't know whether it's appropriate for me to do that, just provide a patch. Or we need to change the API version number again? jst, any comments?

Doug, could you help me to verify the patch on your platform?
Assignee: nobody → alfred.peng
Status: NEW → ASSIGNED
Attachment #213710 - Flags: review?(jst)
Comment on attachment 213710 [details] [diff] [review]
Patchv1

This isn't right. The npruntime API is not a mozilla specific API, and as such we can't use PR types in the API.

There's gotta be a problem with PRUint32 vs uint32_t on BeOS, they should always be defined to be the same type, and the compiler should have no problem converting between them on its own.

We need to dig deeper here. What does inttypes.h on a BeOS box say about uint32_t? And how does it differ from BeOS' PRUint32?
Attachment #213710 - Flags: review?(jst) → review-
inttypes.h is included by nptypes.h (in modules/plugins/base/public) on BeOS, fwiw.
(In reply to comment #3)
...
> 
> Doug, could you help me to verify the patch on your platform?
> 
I'll gladly test any patches but from jst's comments, I don't think I should try this one.
(In reply to comment #4)
...
> 
> We need to dig deeper here. What does inttypes.h on a BeOS box say about
> uint32_t? And how does it differ from BeOS' PRUint32?
> 
I'm afraid I can't answer this question.  I'm hoping biesi or fyysik (sergei) can shed light on this in the morning.
Attachment #213710 - Attachment is obsolete: true
/boot/develop/headers/posix/inttypes.h:
typedef	unsigned int 		u_int32_t;
typedef unsigned int uint32_t;
Sergei, can you preprocess the file "nsJSNPRuntime.cpp" and give the result to us? It can show us what uint32_t and PRUint32 stand for on BeOS platform.

If you are using gcc compiler, you can follow the steps below:
1. cd /boot/home/develop/mozilla/opt-Zeta-ZOOM/modules/plugin/base/src
2. c++ -E -DMOZILLA_INTERNAL_API -DOSTYPE=\"BeOS6.0.1\" -DOSARCH=\"BeOS\" -DBUILD_ID=0000000000  -I../../../../dist/include/xpcom -I../../../../dist/include/xpconnect -I../../../../dist/include/string -I../../../../dist/include/java -I../../../../dist/include/pref -I../../../../dist/include/necko -I../../../../dist/include/caps -I../../../../dist/include/intl -I../../../../dist/include/uconv -I../../../../dist/include/unicharutil -I../../../../dist/include/dom -I../../../../dist/include/gfx -I../../../../dist/include/content -I../../../../dist/include/widget -I../../../../dist/include/mimetype -I../../../../dist/include/oji -I../../../../dist/include/exthandler -I../../../../dist/include/docshell -I../../../../dist/include/windowwatcher -I../../../../dist/include/imglib2 -I../../../../dist/include/layout -I../../../../dist/include/js -I../../../../dist/include/locale -I../../../../dist/include   -I../../../../dist/include/plugin -I../../../../dist/include/nspr    -I../../../../dist/sdk/include    -fPIC   -frtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-multichar -Wno-long-long -pedantic -pipe  -DNDEBUG -DTRIMMED -O3 -march=i686 -mcpu=i686 -frerun-cse-after-loop -frerun-loop-opt   -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -Wp,-MD,.deps/nsJSNPRuntime.pp /boot/home/develop/mozilla/modules/plugin/base/src/nsJSNPRuntime.cpp > nsJSNPRuntime.pre

You may find the exact definitions of uint32_t and PRUint32 in the result file.
output of precompile is at http://sheltonfamily.org/firefox_test/nsJSNPRuntime.pre
alternatively, replace step 2 with "make nsJSNPRuntime.i" ;)
Doug, in your preprocessed file, the definitions of uint32_t and PRUint32 are:
typedef unsigned long uint32_t; line 9055
typedef unsigned int PRUint32; line 938

But Sergei said that the uint32_t should be unsigned int. Can you dig it deeper to get more detail?

On my host(SunOS11 for x86), the types are:
typedef unsigned int PRUint32 ;
typedef unsigned int uint32_t ;

Could you tell me the size of "unsigned int" and "unsigned long"? If your compiler is gcc, can you tell me the version of it?
(In reply to comment #12)
> alternatively, replace step 2 with "make nsJSNPRuntime.i" ;)
> 

Nice. Got it.^_^
(In reply to comment #12)
> alternatively, replace step 2 with "make nsJSNPRuntime.i" ;)
> (In reply to comment #12)
> alternatively, replace step 2 with "make nsJSNPRuntime.i" ;)
> 
that would have saved a bit of time.  nice to know, biesi.  Thanks!

(In reply to comment #13)
> Doug, in your preprocessed file, the definitions of uint32_t and PRUint32 are:
> typedef unsigned long uint32_t; line 9055
> typedef unsigned int PRUint32; line 938
> 
> But Sergei said that the uint32_t should be unsigned int. Can you dig it deeper
> to get more detail?
> 

I'll be happy to dig, though I don't have a shovel :)  Meaning, I don't know the process to follow to get the detail you require.

> On my host(SunOS11 for x86), the types are:
> typedef unsigned int PRUint32 ;
> typedef unsigned int uint32_t ;
> 
> Could you tell me the size of "unsigned int" and "unsigned long"? If your
> compiler is gcc, can you tell me the version of it?
> 

BeOS uses gcc 2.95.3
> > Doug, in your preprocessed file, the definitions of uint32_t and PRUint32 are:
> > typedef unsigned long uint32_t; line 9055
> > typedef unsigned int PRUint32; line 938
> > 
> > But Sergei said that the uint32_t should be unsigned int. Can you dig it deeper
> > to get more detail?
> > 
> 
> I'll be happy to dig, though I don't have a shovel :)  Meaning, I don't know
> the process to follow to get the detail you require.
> 
Maybe sergei ccould help us figure out the reason for "typedef unsigned long uint32_t;" here and "typedef unsigned int uint32_t;" in /boot/develop/headers/posix/inttypes.h.

For the size problem, in gdb prompt, enter:
print sizeof(unsigned int)
print sizeof(unsigned long)
will show us the result.
Attached file BeOS inttypes.h
this is original BeOS inttypes.h

Maybe it helps. BeOS was targeted for two platforms - i86 (using gcc) and PPC (using Metrowerk's mwcc), but PPC target has no importance for us.
Doug, as you are building on Zeta plarform, can you compare Zeta's inttypes.h with attached above BeOS version?
Attached file Zeta inttypes.h
Attachment #213773 - Attachment mime type: application/octet-stream → text/plain
Doug, please attach posix/stdint.h file from Zeta here.

Zeta's inttypes.h is different by form, as instead containing all definitions by self (as R5 version did) it includes stdint.h file.

So we need it to see if there is real difference
hm, or maybe those defs are even in stddef.h
Zeta:
/boot/develop/headers/posix/stdint.h:
typedef unsigned long uint32_t;
btw, long is int in BeOZeta, so actually those uint32_t are equivalent
Attached patch patchSplinter Review
isn't this the easiest way to fix this?
(In reply to comment #24)
> Created an attachment (id=213786) [edit]
> patch
> 
> isn't this the easiest way to fix this?
> 
I think it can fix the problem.

jst, do you accept this fix? Or any other comments?
(In reply to comment #23)
> btw, long is int in BeOZeta, so actually those uint32_t are equivalent
> 
Since they are equivalent, it's strange to have such build error.
Comment on attachment 213786 [details] [diff] [review]
patch

they aren't equivalent, they just have the same size
Attachment #213786 - Flags: superreview?(jst)
Attachment #213786 - Flags: review?(jst)
I've tried gcc 2.95.3 and 3.3.3 to compile the preprocessed file, and got the same error.
On BeOS platform, PRUint32 is defined as unsigned int, while uint32_t is unsigned long defined in system header file. They are different data types in C++. That's the root cause for the build error.
Besides Chris's patch, here is another choice:
       length = 0;
-    } else if (!npobj->_class->enumerate(npobj, &enum_value, &length)) {
+    } else if (!npobj->_class->enumerate(npobj, &enum_value, (uint32_t)&length)) {
jst, what's your opinion?
(In reply to comment #28)
> I've tried gcc 2.95.3 and 3.3.3 to compile the preprocessed file, and got the
> same error.
> On BeOS platform, PRUint32 is defined as unsigned int, while uint32_t is
> unsigned long defined in system header file. They are different data types in
> C++. That's the root cause for the build error.
> Besides Chris's patch, here is another choice:
>        length = 0;
> -    } else if (!npobj->_class->enumerate(npobj, &enum_value, &length)) {
> +    } else if (!npobj->_class->enumerate(npobj, &enum_value,
> (uint32_t)&length)) {
> jst, what's your opinion?
> 
Sorry, should be:
+    } else if (!npobj->_class->enumerate(npobj, &enum_value,
(uint32_t *)&length)) {
Comment on attachment 213786 [details] [diff] [review]
patch

Yeah, this is fine with me. Thanks for digging into this! r+sr=jst (and I'm checking this in now).
Attachment #213786 - Flags: superreview?(jst)
Attachment #213786 - Flags: superreview+
Attachment #213786 - Flags: review?(jst)
Attachment #213786 - Flags: review+
The tree is ok with the patch, so close.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
(In reply to comment #31)
> The tree is ok with the patch, so close.
> 
confirming fix on BeOS
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: