531094 - Inline method crashes GCC 4.5

Status: RESOLVED WONTFIX

(Core :: Disability Access APIs, defect)

Description

[Ehren Metcalfe]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091125 Minefield/3.7a1pre
Build Identifier: 

A known bug in GCC 4.5 causes link errors with inline methods under certain conditions. static const char * ReturnString(nsAString &aString) in nsAccessibleWrap.h is one of these problem methods. See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42171 for details.

Reproducible: Always

Comment 1

[Ehren Metcalfe]

Attachment: deinlining patch

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Aaron isn't around anymore. I can review if you like (:davidb).

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Ehren, will this be needed on FF 3.6 (gecko 1.9.2)?

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 414510 [details] [diff] [review]
deinlining patch

r=me with nit:

Can you add a comment in the header about why this isn't inlined, pointing to the gcc bug?

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Ehren.

I'm cc'ing Ginn, as he is our primary *nix guy.

I'd like some info on when this needs to land (linux distro version?) and on what branches (gecko 1.9.?).

Comment 6

[Ehren Metcalfe]

David, I'm not too sure about 3.6...

My needs are fairly experimental, humph needs it for DXR. Other than that I wouldn't really know.

The updated patch is in the works.

Comment 7

[Ehren Metcalfe]

David, I'm not too sure about 3.6...

My needs are fairly experimental, humph needs it for DXR. Other than that I wouldn't really know.

The updated patch is in the works.

Comment 8

[Ehren Metcalfe]

Attachment: revised patch

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 414515 [details] [diff] [review]
revised patch

Thanks, it is good to have this patch on standby. Right now our repository (trunk and 192) is only open to 1.9.2 critical bugs.

Will this be fixed in gcc?

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 414515 [details] [diff] [review]
revised patch


>+const char * 
>+nsAccessibleWrap::ReturnString(nsAString &aString) {

nit: move the brace on next line

>+  static nsCString returnedString;
>+  returnedString = NS_ConvertUTF16toUTF8(aString);
>+  return returnedString.get();
>+}
>+

>     void SetMaiHyperlink(MaiHyperlink* aMaiHyperlink);
> 
>-    static const char * ReturnString(nsAString &aString) {
>-      static nsCString returnedString;
>-      returnedString = NS_ConvertUTF16toUTF8(aString);
>-      return returnedString.get();
>-    }
>+    // inlining this method causes a known bug in GCC 4.5

I wonder if "inlining" is correct C++ term, just "inline" is associated with methods defined with "inline" keyword.

>+    // see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42171 for details
>+    static const char * ReturnString(nsAString &aString);

Comment 11

[Ehren Metcalfe]

Attachment: style/language fixup

I also considered linking to http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41611 in the header which more accurately explains the problem. Since http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42171 is marked as a duplicate (and links to 41611) I figured it wasn't necessary.

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 414683 [details] [diff] [review]
style/language fixup

I think it's not necessary to ask review for nit changes like these. Any way if you like, r=me.

Btw, I still not sure about "inline" term because inline means compiler should try to not generate function call, but here it's just declaration and definition of the method are coincided. Probably in this case compiler also will try to not generate method call and then it would be inline method, I don't know. But I can see in bug you refer to "inline" term is used in this meaning. So it's ok I think.

Comment 13

[Ehren Metcalfe]

hmm, the revised patch may be more inaccurate since the member function was never actually declared 'inline'. I would bet that if it was declared inline and the definition was moved out of the body you'd still get the bug though.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> hmm, the revised patch may be more inaccurate since the member function was
> never actually declared 'inline'. I would bet that if it was declared inline
> and the definition was moved out of the body you'd still get the bug though.

Ok. Let's leave it as is.

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

I think this needs to be fixed in gcc..
(see P1 bug:http://gcc.gnu.org/bugzilla/show_bug.cgi?id=41611)

I'm grateful for the patch which we'll need if the gcc issue stays unresolved.

Comment 16

[Ehren Metcalfe]

Attachment: comment language cleanup

just a quick fixup for the comment. Interestingly, I checked out the c++ standard at http://www.open-std.org/JTC1/SC22/WG21/docs/papers/2005/n1905.pdf which says at 9.3.2 "A member function may be defined in its class definition, in which case it is an inline member function"

Comment 17

[Ginn Chen]

Since it is a known bug of GCC 4.5, I suggest we close it as INVALID here.

Comment 18

[alexander :surkov (:asurkov)]

The question should be is GCC4.5 is used to build Gecko 1.9.3 officially. Is it?

Comment 19

[Benjamin Smedberg]

GCC 4.5 is still in beta phase. The bug will be fixed before final (it's listed as a P1 regression in their bug tracker). And when it is released (probably early next year) it will certainly be used by various Linux distros and perhaps Mozilla to do official releases.

Comment 20

[alexander :surkov (:asurkov)]

So, wontfix then?

Comment 21

[David Humphrey (:humph)]

wontfix is going to mean that any analysis/dxr work I do from here on is --disable-accessiblity, which is not my first choice.

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

We could reopen, land, and leave open or file a new bug to revert.

Also, do we really need this method to be inline?

Comment 23

[Ehren Metcalfe]

This issue's been fixed as of GCC revision 154885. I tried a build just to be safe and it works, so the patch is no longer necessary.

Comment 24

[alexander :surkov (:asurkov)]

So, wonfitx then again?

Comment 25

[Ehren Metcalfe]

Invalid maybe? I'm not too sure.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> Invalid maybe? I'm not too sure.

Technically GCC crashed because of inline method, that's true. The bug description/summary is correct. It's not our fault and therefore we probably don't want to fix it. However I'm happy with either resolution.

Comment 27

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

INVALID: bug is misreported, or we cannot do anything to fix it (such as an upstream problem with no workaround)

WONTFIX: the bug as filed could be fixed by a patch to our code, but we expressly do not want such a patch.

I think this is WONTFIX.