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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: doug, Assigned: alfred.peng)
Details
Attachments
(4 files, 1 obsolete file)
2.35 KB,
text/plain
|
Details | |
1.41 KB,
text/plain
|
Details | |
969 bytes,
text/plain
|
Details | |
821 bytes,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•18 years ago
|
Component: General → Plug-ins
Product: Firefox → Core
Version: unspecified → Trunk
Reporter | ||
Comment 1•18 years ago
|
||
Comment 2•18 years ago
|
||
as far I understand, using PR* types everywhere in Mozilla is preferable. wondering where from that uint32_t passed in.
Assignee | ||
Updated•18 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 3•18 years ago
|
||
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?
Comment 4•18 years ago
|
||
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-
Comment 5•18 years ago
|
||
inttypes.h is included by nptypes.h (in modules/plugins/base/public) on BeOS, fwiw.
Reporter | ||
Comment 6•18 years ago
|
||
(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.
Reporter | ||
Comment 7•18 years ago
|
||
(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.
Assignee | ||
Updated•18 years ago
|
Attachment #213710 -
Attachment is obsolete: true
Comment 8•18 years ago
|
||
/boot/develop/headers/posix/inttypes.h: typedef unsigned int u_int32_t;
Comment 9•18 years ago
|
||
typedef unsigned int uint32_t;
Assignee | ||
Comment 10•18 years ago
|
||
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.
Reporter | ||
Comment 11•18 years ago
|
||
output of precompile is at http://sheltonfamily.org/firefox_test/nsJSNPRuntime.pre
Comment 12•18 years ago
|
||
alternatively, replace step 2 with "make nsJSNPRuntime.i" ;)
Assignee | ||
Comment 13•18 years ago
|
||
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?
Assignee | ||
Comment 14•18 years ago
|
||
(In reply to comment #12) > alternatively, replace step 2 with "make nsJSNPRuntime.i" ;) > Nice. Got it.^_^
Reporter | ||
Comment 15•18 years ago
|
||
(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
Assignee | ||
Comment 16•18 years ago
|
||
> > 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.
Comment 17•18 years ago
|
||
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.
Comment 18•18 years ago
|
||
Doug, as you are building on Zeta plarform, can you compare Zeta's inttypes.h with attached above BeOS version?
Reporter | ||
Comment 19•18 years ago
|
||
Updated•18 years ago
|
Attachment #213773 -
Attachment mime type: application/octet-stream → text/plain
Comment 20•18 years ago
|
||
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
Comment 21•18 years ago
|
||
hm, or maybe those defs are even in stddef.h
Comment 22•18 years ago
|
||
Zeta: /boot/develop/headers/posix/stdint.h: typedef unsigned long uint32_t;
Comment 23•18 years ago
|
||
btw, long is int in BeOZeta, so actually those uint32_t are equivalent
Comment 24•18 years ago
|
||
isn't this the easiest way to fix this?
Assignee | ||
Comment 25•18 years ago
|
||
(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?
Assignee | ||
Comment 26•18 years ago
|
||
(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 27•18 years ago
|
||
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)
Assignee | ||
Comment 28•18 years ago
|
||
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?
Assignee | ||
Comment 29•18 years ago
|
||
(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 30•18 years ago
|
||
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+
Assignee | ||
Comment 31•18 years ago
|
||
The tree is ok with the patch, so close.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 32•18 years ago
|
||
(In reply to comment #31) > The tree is ok with the patch, so close. > confirming fix on BeOS
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•