275004 - Mozilla crashes on startup when compiled with IBM XL C++ v7

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Philip K. Warren]

With the new version of the compiler, Mozilla crashes on startup in the
xptcstubs code. It seems that the layout of the stack changed in the v7
compiler, and so the position of the 'this' pointer has changed. I was able to
get it to work by making the following two changes to xptcstubs_asm_ppc_aix.s:


-    lwz     r3,104(sp)      # Get the 'original' r3
+    lwz     r3,88(sp)       # Get the 'original' r3

and:

-    addi    r5,sp,312       # pointer to callers args area, beyond r3-r10
mapped range
+    addi    r5,sp,296       # pointer to callers args area, beyond r3-r10
mapped range

I would like to talk to the compiler team to ensure this is the correct fix to
make for v7, and then I will work on a patch to check in to Mozilla (will need
autoconf test most likely and another xptcstubs assembler file for v7 compiler).

Comment 1

[Darin Fisher]

> ...and another xptcstubs assembler file for v7 compiler).

maybe just use #ifdefs for those two changes?

Comment 2

[Philip K. Warren]

(In reply to comment #1)
> maybe just use #ifdefs for those two changes?

I think I tried that before but I couldn't put ifdefs in the assembler file 
itself.

Comment 3

[Jim Dunn]

Definitely double check the stack pointer.

I have no idea if you will run into any issues but I sort of remember
going back 2 stack frames with the original code, so you probably
want to make sure you are doing the same in the new code.  But who 
knows.

As for ifdefs, I think you will have to make new assembly files and
put ifdefs in the Makefile...

good luck ;-)

Comment 4

[Philip K. Warren]

(In reply to comment #3)
> I have no idea if you will run into any issues but I sort of remember
> going back 2 stack frames with the original code, so you probably
> want to make sure you are doing the same in the new code.  But who 
> knows.

I stepped through in the debugger with both the old and new compiled code, and I
see the following:

Old compiler:
'this' pointer found at sp+104, sp+112, sp+164

New compiler:
'this' pointer found at sp+88, sp+96, sp+148

Comment 5

[Jim Dunn]

ok, then it looks like the line
+    lwz     r3,88(sp)       # Get the 'original' r3
will be fine.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

so apps compiled with the new version of the compiler are incompatible with C++
libraries compiled with the old ver?

Comment 7

[Philip K. Warren]

(In reply to comment #6)
> so apps compiled with the new version of the compiler are incompatible with 
C++
> libraries compiled with the old ver?

I don't believe so. The name mangling is still the same. What I've heard is 
that when upgrading the compiler, customers only need to have the latest C++ 
runtime installed on their systems.

I think the best solution here is to follow the model of the mips/darwin stubs 
code and generate all of the Stubs* functions in the assembly code instead of 
in the C++ code. This will remove the need to know anything about the stack 
layout, as the 'this' pointer will be passed directly in the r3 register. I 
have a patch which implements this method which I'll just need to test for all 
of the different configurations (32-bit, 64-bit, ibm objmodel, compat 
objmodel).

Comment 8

[Philip K. Warren]

Attachment: Patch v1

This changes the xptcstubs code on AIX to use m4 to generate all of the Stubs*
functions in the assembly code, which allows us to branch directly to the
SharedStub function and not have to do any calculations with the stack size. I
have tested this code when built with the IBM and the Compat object models, and
both work great.

Comment 9

[The Written Word]

I manually modified
mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_ppc_aix.s with the
two-line update in comment #1 and built on AIX 5.2 with the v7.0.0.1 (November
2004 patchset) compiler but I get:
  Illegal instruction(coredump)

I build with CFLAGS="-g" CXXFLAGS="-g". gdb didn't show anything interesting:
  $ gdb-6.3 /opt/TWWfsw/firefox10/bin/firefox core
  gdb> bt
  #0  0xd0062514 in pthread_kill () from /usr/lib/libpthreads.a(shr_xpg5.o)
  #1  0xd0061f9c in _p_raise () from /usr/lib/libpthreads.a(shr_xpg5.o)
  #2  0x1002dbb8 in nsProfileLock::FatalSignalHandler (signo=4)
      at nsProfileLock.cpp:205
  #3  0x000041ac in ?? ()
  #4  0x000041ac in ?? ()

I'll try your patch. Have you tested on AIX 5.2 with the 7.0.0.1 compiler?

Comment 10

[Philip K. Warren]

(In reply to comment #9)
> I manually modified
> mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_ppc_aix.s with the
> two-line update in comment #1 and built on AIX 5.2 with the v7.0.0.1 
(November
> 2004 patchset) compiler but I get:
>   Illegal instruction(coredump)
> I build with CFLAGS="-g" CXXFLAGS="-g". gdb didn't show anything interesting:
>   $ gdb-6.3 /opt/TWWfsw/firefox10/bin/firefox core
>   gdb> bt
>   #0  0xd0062514 in pthread_kill () from /usr/lib/libpthreads.a(shr_xpg5.o)
>   #1  0xd0061f9c in _p_raise () from /usr/lib/libpthreads.a(shr_xpg5.o)
>   #2  0x1002dbb8 in nsProfileLock::FatalSignalHandler (signo=4)
>       at nsProfileLock.cpp:205
>   #3  0x000041ac in ?? ()
>   #4  0x000041ac in ?? ()
> I'll try your patch. Have you tested on AIX 5.2 with the 7.0.0.1 compiler?

Yes - actually I have tested on AIX 5.2 ML04 with the 7.0.0.1 compiler, and on 
AIX 5.1 Gold with the 6.0.0.3 compiler. Do you have any specific steps to 
reproduce this crash, or does it just happen on startup? Is this a GTK1 or 
GTK2 build? Is this a trunk build?

Comment 11

[The Written Word]

$ oslevel -r
5200-01
$ lslpp -L bos.rte.libc
  Fileset                      Level  State  Type  Description (Uninstaller)
  ----------------------------------------------------------------------------
  bos.rte.libc              5.2.0.41    C     F    libc Library
$ lslpp -L bos.adt.include
  Fileset                      Level  State  Type  Description (Uninstaller)
  ----------------------------------------------------------------------------
  bos.adt.include           5.2.0.41    C     F    Base Application Development
                                                   Include Files
$ lslpp -L vacpp.cmp.core
  Fileset                      Level  State  Type  Description (Uninstaller)
  ----------------------------------------------------------------------------
  vacpp.cmp.core             7.0.0.1    C     F    IBM XL C/C++ Compiler

I built firefox-1.0 against atk-1.6.1, freetype-2.1.4, glib-2.4.4, gtk+-2.4.4,
libIDL-0.8.3, pango-1.4.0, libpng-1.2.4, jpeg-6b as follows:
  $ cd /opt/build
  $ gtar jxf firefox-1.0-source.tar.bz2
  $ cd /opt/build/mozilla
  [apply patch in comment #8]
  $ BUILD_OFFICIAL=1 MOZILLA_OFFICIAL=1 MOZ_PHOENIX=1 \
  CC=xlc_r CFLAGS="-g" CXX=xlC_r CXXFLAGS="-g" \
  LDFLAGS="-blibpath:/opt/TWWfsw/libpng12/lib:/opt/TWWfsw/jpeg/lib:\
  /opt/TWWfsw/zlib11/lib:/usr/lib -brtl" \
  PKG_CONFIG_PATH=/opt/TWWfsw/fcpackage22/lib/pkgconfig:\
  /opt/TWWfsw/libglib24/lib/pkgconfig:/opt/TWWfsw/libgtk24/lib/pkgconfig:\
  /opt/TWWfsw/libpango14/lib/pkgconfig:/opt/TWWfsw/libatk16/lib/pkgconfig:\
  /opt/TWWfsw/libidl08/lib/pkgconfig:/opt/TWWfsw/libttf21/lib/pkgconfig:\
  /opt/TWWfsw/gettext014/lib/pkgconfig" \
  ./configure --enable-default-toolkit=gtk2 \
  --with-ft-prefix=/opt/TWWfsw/libttf21 --enable-crypto --disable-ldap \
  --disable-mailnews --disable-strip \
  --enable-extensions=default,-irc,-venkman,-wallet --disable-debug \
  --enable-xft --disable-gnomevfs --enable-single-profile \
  --disable-profilesharing --disable-installer --enable-mathml \
  --enable-jsd --enable-xpctools --enable-oji --disable-tests
  $ BUILD_OFFICIAL=1 MOZILLA_OFFICIAL=1 MOZ_PHOENIX=1 gmake
  $ cd /opt/build/mozilla/dist/bin
  $ LIBPATH=/opt/build/mozilla/dist/bin ./firefox-bin

I'm building now. We do have a test machine we can upgrade to 5200-04 if needed.

Comment 12

[The Written Word]

Ok, on the ML01 build machine, I get the same crash. However, I upgraded the
test machine to ML02 and the *same* binary worked. So, looks like an OS issue.
I'll investigate some more.

Comment 13

[The Written Word]

On the ML02 machine, before upgrading to the latest xlC.aix50.rte and xlC.rte
runtime that are part of the Nov2004 xlC v7 patch set, I upgraded bos.rte.libc
to  5.2.0.41 and bos.adt.include to 5.2.0.41. The necessary dependendies were
also installed:
  bos.64bit                 5.2.0.43    A     F    Base Operating System 64 bit
  bos.adt.include           5.2.0.41    A     F    Base Application Development
  bos.adt.prof              5.2.0.41    A     F    Base Profiling Support
  bos.adt.syscalls          5.2.0.41    A     F    System Calls Application
  bos.diag.com              5.2.0.30    A     F    Common Hardware Diagnostics
  bos.diag.rte              5.2.0.30    A     F    Hardware Diagnostics
  bos.diag.util             5.2.0.30    A     F    Hardware Diagnostics Utilities
  bos.mp                    5.2.0.45    A     F    Base Operating System
  bos.mp64                  5.2.0.45    A     F    Base Operating System 64-bit
  bos.net.ipsec.rte         5.2.0.30    A     F    IP Security
  bos.net.nfs.client        5.2.0.30    A     F    Network File System Client
  bos.net.tcp.client        5.2.0.40    A     F    TCP/IP Client Support
  bos.net.tcp.server        5.2.0.40    A     F    TCP/IP Server
  bos.perf.diag_tool        5.2.0.40    A     F    Performance Diagnostic Tool
  bos.perf.libperfstat      5.2.0.40    A     F    Performance Statistics Library
  bos.perf.perfstat         5.2.0.40    A     F    Performance Statistics
  bos.perf.tools            5.2.0.40    A     F    Base Performance Tools
  bos.perf.tune             5.2.0.40    A     F    Performance Tuning Support
  bos.rte                   5.2.0.41    A     F    Base Operating System Runtime
  bos.rte.boot              5.2.0.40    A     F    Boot Commands
  bos.rte.control           5.2.0.40    A     F    System Control Commands
  bos.rte.libc              5.2.0.41    A     F    libc Library
  bos.rte.libpthreads       5.2.0.30    A     F    libpthreads Library
  bos.rte.serv_aid          5.2.0.30    A     F    Error Log Service Aids
  bos.rte.tty               5.2.0.30    A     F    Base TTY Support and Commands
  bos.sysmgt.serv_aid       5.2.0.40    A     F    Software Error Logging and
  bos.sysmgt.trace          5.2.0.30    A     F    Software Trace Service Aids
  bos.up                    5.2.0.45    A     F    Base Operating System
  devices.chrp.base.rte     5.2.0.40    A     F    RISC PC Base System Device
  devices.chrp.pci.rte      5.2.0.40    A     F    PCI Bus Software (CHRP)
                            5.2.0.40    A     F    Base CHRP LPAR Devices
                            5.2.0.30    A     F    Common Ethernet Software
                            5.2.0.40    A     F    Common IBM FC Software
  devices.pci.14100401.rte  5.2.0.30    A     F    Gigabit Ethernet-SX PCI
                            5.2.0.30    A     F    IBM PCI SCSI RAID Adapter
  devices.pci.14106802.rte  5.2.0.30    A     F    Gigabit Ethernet-SX PCI-X
  devices.pci.14106902.rte  5.2.0.30    A     F    10/100/1000 Base-TX PCI-X
  devices.pci.14108802.rte  5.2.0.30    A     F    2-Port Gigabit Ethernet-SX
  devices.pci.14108902.rte  5.2.0.30    A     F    2-Port 10/100/1000 Base-TX
  devices.pci.1410ff01.rte  5.2.0.30    A     F    10/100 Mbps Ethernet PCI
  devices.pci.22100020.rte  5.2.0.30    A     F    IBM PCI Ethernet Adapter
  devices.pci.23100020.rte  5.2.0.30    A     F    IBM PCI 10/100 Ethernet
  devices.tty.rte           5.2.0.30    A     F    TTY Device Driver Support
  perfagent.tools           5.2.0.40    A     F    Local Performance Analysis &

After installation of the above, I get the "Illegal Instruction" coredump on the
ML02 machine. So, one of the above filesets is causing the problem.

Comment 14

[The Written Word]

Philip, what version of bos.rte.libc is on your ML04 machine?

Comment 15

[Philip K. Warren]

(In reply to comment #14)
> Philip, what version of bos.rte.libc is on your ML04 machine?

5.2.0.50. There was recently a regression in the dlerror() function in libc, 
which is fixed in APAR IY65062. This caused a crash in the gmodule code in 
glib2. We added a temporary fix to the glib2 RPM provided on the AIX Toolbox 
to work around this problem, but since you are using a glib2 built from 
source, you are most likely running into this issue. Do you get any sort of a 
stack trace?

Comment 16

[The Written Word]

After installing ML02, I installed bos.rte.libc 5.2.0.41 and 5.2.0.42. I
encountered the crash in both. gdb doesn't show anything more than what I gave
in comment #9. Does idebug have a command-line-level debugger like gdb? I don't
see one.

I did try bos.rte.libc 5.2.0.50 and 5.2.0.51 and both worked. So, you might be
right. We are using glib-2.4.4 though. I reviewed glib-2.2.3-aix.patch from
ftp://ftp.software.ibm.com:/aix/freeSoftware/aixtoolbox/PATCHES but don't see
anything abvious about the dlerror() workaround you mentioned. Is the patch
containing the workaround at some other location?

Comment 17

[Philip K. Warren]

(In reply to comment #16)
> After installing ML02, I installed bos.rte.libc 5.2.0.41 and 5.2.0.42. I
> encountered the crash in both. gdb doesn't show anything more than what I 
gave
> in comment #9. Does idebug have a command-line-level debugger like gdb? I 
don't
> see one.

The fileset bos.adt.debug ships the 'dbx' command line compiler, which is 
similar to gdb.

> I did try bos.rte.libc 5.2.0.50 and 5.2.0.51 and both worked. So, you might 
be
> right. We are using glib-2.4.4 though. I reviewed glib-2.2.3-aix.patch from
> ftp://ftp.software.ibm.com:/aix/freeSoftware/aixtoolbox/PATCHES but don't see
> anything abvious about the dlerror() workaround you mentioned. Is the patch
> containing the workaround at some other location?

I believe the only change was to change all of the fetch_dlerror calls in 
gmodule-ar.c to pass TRUE.

Comment 18

[The Written Word]

Ok, here's the dbx backtrace:
  $ dbx ./firefox-bin core
  dbx> where
pthread_kill(??, ??) at 0xd0062514
warning: Unable to access address 0xfffffffffffffffc from core
warning: Unable to access address 0xfffffffffffffffc from core
warning: Unable to access address 0xfffffffffffffffc from core
warning: Unable to access address 0xfffffffffffffffc from core
_p_raise(??) at 0xd0061f98
unnamed block $b50, line 205 in "nsProfileLock.cpp"
FatalSignalHandler(int)(0x4), line 205 in "nsProfileLock.cpp"
.() at 0x0
g_module_open(??, ??) at 0xd2fb67a4
_gdk_pixbuf_load_module(??, ??) at 0xd2e273f0
gdk_pixbuf_new_from_file(??, ??) at 0xd2e29164
unnamed block $b1386, line 2613 in "nsWindow.cpp"
SetWindowIconList(const nsCStringArray&)(0x2069cf58, 0x2ff21d50), line 2613 in
"nsWindow.cpp"
SetDefaultIcon()(0x2069cf58), line 2659 in "nsWindow.cpp"
NativeCreate(nsIWidget*,void*,const
nsRect&,nsEventStatus(*)(nsGUIEvent*),nsIDeviceContext*,nsIAppShell*,nsIToolkit*,nsWidgetInitData*)(0x2069cf58,
0x0, 0x0, 0x2ff21ef8, 0x205b35b8, 0x0, 0x20693538, 0x0), line 2138 in "nsWindow.cpp"
nsWindow.Create(nsIWidget*,const
nsRect&,nsEventStatus(*)(nsGUIEvent*),nsIDeviceContext*,nsIAppShell*,nsIToolkit*,nsWidgetInitData*)(0x2069cf58,
0x0, 0x2ff21ef8, 0x205b35b8, 0x0, 0x20693538, 0x0, 0x2ff22140), line 309 in
"nsWindow.cpp"
Initialize(nsIXULWindow*,nsIAppShell*,nsIURI*,int,int,int,int,int,nsWidgetInitData&)(0x2069c7c8,
0x0, 0x20693538, 0x2069bbd8, 0x0, 0x0, 0x64, 0x64), line 277 in
"nsWebShellWindow.cpp"
unnamed block $b1763, line 858 in "nsAppShellService.cpp"
JustCreateTopWindow(nsIXULWindow*,nsIURI*,int,int,unsigned
int,int,int,int,nsIXULWindow**)(0x206934c8, 0x0, 0x2069bbd8, 0x0, 0x0, 0xffe,
0x64, 0x64), line 858 in "nsAppShellService.cpp"
unnamed block $b1752, line 459 in "nsAppShellService.cpp"
CreateHiddenWindow()(0x206934c8), line 459 in "nsAppShellService.cpp"
unnamed block $b14, line 1824 in "nsAppRunner.cpp"
unnamed block $b13, line 1824 in "nsAppRunner.cpp"
xre_main(int,char**,const nsXREAppData*)(0x1, 0x2ff22980, 0x200007c0), line 1824
in "nsAppRunner.cpp"
main(argc = 1, argv = 0x2ff22980), line 58 in "nsBrowserApp.cpp"

Looks like it's dying in a call from:
  g_module_open(??, ??) at 0xd2fb67a4
which is in line with what you mentioned earlier.

I think the safest bet is for us to install bos.rte.libc 5.2.0.50.

Does AIX 5.1 have a similar problem? It'll be a few days before I can upgrade
the compiler on our 5.1 machine.

Comment 19

[Perry Smith]

I hit this bug.  I am running AIX 5.3.0.50 bos.rte.  The xlc I have is version 8.  The firefox I am trying to compile and run is firefox 2.0.0.3.

I installed the patch provided by Philip and it worked for me.  I'm not dying in a different place but I think it solved this issue.

HTH

Comment 20

[Philip K. Warren]

This patch does not appear to fix the 64-bit compiled version of the browser. I think a similar change would be required to get that working.

Comment 21

[Shailen]

Attachment: Patch V2

Created a patch similar to that of 32-bit for 64-bit compiled version of the browser.

Comment 22

[Benjamin Smedberg]

Comment on attachment 291637 [details] [diff] [review]
Patch V2

This is ugly... can't you just hack genstubs.pl to avoid putting the C++ comments in xptcstubsdef.inc?

Comment 23

[Shailen]

Attachment: Patch V3

Modified genstubs.pl to avoid putting the C++ comments in xptcstubsdef.inc

Comment 24

[Benjamin Smedberg]

Comment on attachment 295069 [details] [diff] [review]
Patch V3

You changed genstubs.pl... why do you need to preprocess ./xptcstubsdefaix.inc now? Clearing request on the assumption that you don't need to do this... please re-request review on this patch if I misunderstood.

Comment 25

[Shailen]

 I guess I have misunderstood your review comment #22. I thought I would have to modify genstubs.pl to remove the c++ comments in xptcstubsdef.inc and therefore I had created a new patch to include this change.

Can you please advise what am I missing in the patch V2? I will take care of that in the next patch and submit again.

Comment 26

[Benjamin Smedberg]

You modified genstub.pl... but then you kept the preprocessing of that file to xptcstubsdefaix.inc, which should be unnecessary.

Comment 27

[Shailen]

Attachment: Patch V4

Modified the patch to include the resulting changes to xptcstubsdef.inc after preprocessing the genstubs.pl to remove C++ comments in xptcstubsdef.inc

Comment 28

[Benjamin Smedberg]

Comment on attachment 295736 [details] [diff] [review]
Patch V4

>+# The AIX assembler chokes on C++ comments in xptcstubdef.inc file, so we
>+# need to preprocess the file first.
>+ifeq ($(OS_ARCH),AIX)
>+ifdef HAVE_64BIT_OS
>+xptcstubs_asm_ppc_aix64.o: xptcstubs_asm_ppc_aix64.s.m4 $(PUBLIC)/xptcstubsdef.inc Makefile
>+	$(CC) -E $(PUBLIC)/xptcstubsdef.inc > ./xptcstubsdefaix.inc
>+	m4 -DAIX_OBJMODEL=$(AIX_OBJMODEL) $(INCLUDES) -I. $< > ./xptcstubs_asm_ppc_aix64.s && \
>+	$(AS) -o $@ $(ASFLAGS) $(AS_DASH_C_FLAG) ./xptcstubs_asm_ppc_aix64.s
>+	$(RM) ./xptcstubs_asm_ppc_aix64.s
>+	$(RM) ./xptcstubsdefaix.inc
>+else
>+xptcstubs_asm_ppc_aix.o: xptcstubs_asm_ppc_aix.s.m4 $(PUBLIC)/xptcstubsdef.inc Makefile
>+	$(CC) -E $(PUBLIC)/xptcstubsdef.inc > ./xptcstubsdefaix.inc
>+	m4 -DAIX_OBJMODEL=$(AIX_OBJMODEL) $(INCLUDES) -I. $< > ./xptcstubs_asm_ppc_aix.s && \
>+	$(AS) -o $@ $(ASFLAGS) $(AS_DASH_C_FLAG) ./xptcstubs_asm_ppc_aix.s
>+	$(RM) ./xptcstubs_asm_ppc_aix.s
>+	$(RM) ./xptcstubsdefaix.inc
>+endif
>+endif

You still haven't removed this hunk, which I believe is/should be completely unneecessary. Can you explain why you still need it?

Comment 29

[Shailen]

Attachment: Patch V5

Sorry, I forgot to remove preprocessing part from Makefile.in

Made the above changes and hence attaching a new patch.

Comment 30

[timeless]

Comment on attachment 296310 [details] [diff] [review]
Patch V5

sorry. I want an explanation as to why you are changing genstubs.pl and xptcstubsdef.inc

Are you somehow claiming that xpcstubsdef.inc is not a generated file?

Comment 31

[timeless]

ok, short explanation of my problems.
1. The comment being removed /* ... */ was a C comment, not really a C++ comment.
2. I don't like losing the comment, I feel it's important. The file *is* generated and I don't want people to think that they can change it directly. And perhaps more importantly, having the line in blame will show them that they can't add C comments if they decide they want to add a comment.

I've asked sjain to add STUB_COMMENT("...") to replace /*...*/

This of course requires adding #define STUB_COMMENT() to the other ports (or however it's done for the other ports) next to STUB_ENTRY, but I feel it's better than losing the hint.

Comment 32

[David Bradley]

Can't or shouldn't we treat the comment issue as another bug? I think making this platform "work" is this bug and it would look a little odd to have hunks sprinkled through out other platforms for a platform specific bug. If the STUB_COMMENT addition is trivial and comes quick then I'd say wait, else I'd say lets get this platform working and then address the comment issue.

Comment 33

[Shailen]

timeless: I have checked and I couldnt find any option to have STUB_COMMENT(" ") which can be invoked directly in .inc file.

Also I am not sure how will I be testing the change in other platforms if I define STUB_COMMENT and invoke.

Comment 34

[David Bradley]

Right, it doesn't exist, he was saying you were going to create it. But you're right, such a change isn't trivial to prove across all platforms. Not sure if there's a way/process to utilize the tinderbox for such changes. That's why I put out the suggestion to deal with it as a separate bug, since it really is and it's probably not trivial to address.

Comment 35

[Shailen]

David Bradley: I like your suggestion to deal stub comments as a separate bug and it would be nice to have the real patch for this bug checked-in at the earliest.

Both Patch V2 and Patch V5 have the fix for this bug. 

Patch V2 did not approved as the reviewer bsmedberg thinks it is the ugly way to handle .inc with C/C++ comments. 

Patch V5 : It did remove the C/C++ comments in .inc but did not get approved as the reviewer 'timeless' thinks the comments shouldn't have been removed.

Looking for advice as what needs to be done next.

Comment 36

[timeless]

Comment on attachment 296310 [details] [diff] [review]
Patch V5

ok. instead of removing the lines from genstubs.pl, wrap an if around it.

+# Disabled for bug 275004 - followup to fix is Bug ...
+my $warn_inc_is_generated = 0;
+if ($warn_inc_is_generated) {
 print OUTFILE "/* generated file - DO NOT EDIT */\n\n";
 print OUTFILE "/* includes ",$entry_count," stub entries, and ",
               $sentinel_count," sentinel entries */\n\n";
+}

that way the line is there and people can see that we're not trying to remove it permanently.

Yes, this requires filing the followup bug before committing so you get a bug number :).

Comment 37

[Shailen]

Attachment: Patch V6

Modified the patch to enter the follow up bug for removing the comments from xptcstubsdef.inc

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 305726 [details] [diff] [review]
Patch V6

a=beltzner for 1.9b4

Comment 39

[Robert Sayre]

why in the world would we approve this patch for b4? are we planning on shipping AIX tarballs?

Comment 40

[timeless]

Comment on attachment 305726 [details] [diff] [review]
Patch V6

mozilla/xpcom/reflect/xptcall/public/genstubs.pl 	1.10
mozilla/xpcom/reflect/xptcall/public/xptcstubsdef.inc 	1.5
mozilla/xpcom/reflect/xptcall/src/md/unix/Makefile.in 	1.100
mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_ppc_aix.s 	1.6
mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_ppc_aix.s.m4 	1.1
mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_ppc_aix64.s 	1.2
mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_asm_ppc_aix64.s.m4 	1.1
mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_ppc_aix.cpp 	1.10
mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_ppc_aix64.cpp 	1.3