Closed Bug 590996 Opened 14 years ago Closed 7 years ago

Build of libffi broken by valid linker switch

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED INCOMPLETE
Tracking Status
blocking2.0 --- -

People

(Reporter: swsnyder, Assigned: Mitch)

References

Details

(Whiteboard: [patchlove])

Attachments

(1 file, 2 obsolete files)

The build of libffi is broken if LDFLAGS contains the VS2005 linker switch -LTCG.  

The reason is that libtool believes that -L is always followed by a path to required libraries, and chokes when it sees those trailing "TCG" characters.  It aborts compilation, complaining that path TCG can't be found.

I marked this bug as OS == "Windows XP" but it is really specific to the Visual Studio compiler.

Seen on Firefox 4.0b4 release tarball, built with VS2005 on WinXP.
Attachment #469552 - Attachment is patch: true
Attachment #469552 - Attachment mime type: application/octet-stream → text/plain
I've just hit the same build error, and was pleased to find your patch attached. Thanks!

Did you mean to request review?
(In reply to comment #1)
> Did you mean to request review?

Maybe this patch is the best course, but it seems kind of clunky to me.  (What other valid switches will be inappropriately matched in the libtool gen?)  There's probably a better way, but I'm not very familiar with the details of libtool.

Given my uncertainty it seemed best to create this bug just to get it on the record, and to provided this patch to at least work around the problem.
See also: Bug 600237
I think this should block 2.0 as it breaks the build in a configuration (MS VS 2005) that, I think, is still supported. And if it doesn't block, it probably still deserves some attention to at least determine the exact set of build configurations that no longer work.
blocking2.0: --- → ?
Our tinderboxes run VS2005, I believe, so I'm not quite sure what's different about this setup.  If you're saying "there exist ways to change your setup and make configurations that don't work", then that's true and I could see taking patches for them, but I can't see us blocking.
blocking2.0: ? → -
I was under the, apparently false, assumption that tinderboxes had VS2005 installed along newer versions and used VS2008 or 2010 to build on the mozilla-central tree. Sorry about that.

I'm certainly not looking for ways to change my setup to make configurations that don't work. Both my local Windows virtual machine and the Windows virtual machine used for Instantbird's buildbot slave couldn't compile the code from mozilla-central without applying the patch attached here.
These 2 virtual machines have Windows XP (service pack 2 I think) with Visual Studo 2005 Pro SP1.
Attached patch Patch (obsolete) — Splinter Review
This is based on Steve's patch and works around -LARGEADDRESSAWARE too, which is apparently a problem: https://bugzilla.mozilla.org/show_bug.cgi?id=556382#c30

Completely untested.
Attachment #491822 - Flags: feedback?(florian)
Comment on attachment 491822 [details] [diff] [review]
Patch

A Windows build of Instantbird has just completed successfully with this patch.
Attachment #491822 - Flags: feedback?(florian) → feedback+
Attachment #491822 - Flags: review?(shaver)
Comment on attachment 491822 [details] [diff] [review]
Patch

Lets get this into the tree.
Attachment #491822 - Flags: review?(shaver) → review+
Patch 491822 works fine. But I also got it to work just by disabling LARGEADDRESSAWARE.  Disabling LARGEADDRESSAWARE is enough and disabling LTCG is not required.

# Avoid matching MSVC linker option.
-LARGEADDRESSAWARE)
continue
;;


The following is not required since the issue fundamentally is because of LARGEADDRESSAWARE:
# Avoid matching MSVC linker option.
-LTCG)
continue
;;
(In reply to comment #10)
> Patch 491822 works fine. But I also got it to work just by disabling
> LARGEADDRESSAWARE.  Disabling LARGEADDRESSAWARE is enough and disabling LTCG is
> not required.

It was required when this bug was filed, and probably still is in some configurations. The LARGEADDRESSAWARE issue appeared later when bug 556382 landed.
ok. So just as a note, I was building ff4b8 on Win7 with vc9 and just avoiding LARGEADDRESSAWARE worked for me.
Assignee: general → mitchell.field
Mitch, is there a reason this hasn't landed yet?
Just got bit by this for -LARGEADDRESSAWARE on Windows 7.
Blocks: 657571
I was hit by this last month, and was easily able to workaround the problem by changing -LARGEADDRESSAWARE and -LTCG to -largeaddressaware and -ltcg respectively in config files and LDFLAGS (if applicable). libiff is case-sensitive with its switches, but the MSVC linker doesn't care.  When using -largeaddressaware and -ltcg, libiff will ignore them (only cares about capital '-L') and they will get passed to the MSVC linker as normal.
khuey, any reason this can't land?
Whiteboard: [patchlove]
khuey, do you know the status of this patch, and especially why it hasn't landed?
Flags: needinfo?(khuey)
Similar hilarity from current builds:
cl : Command line warning D9002 : ignoring unknown option '-O3'
cl : Command line warning D9002 : ignoring unknown option '-OP'
cl : Command line warning D9002 : ignoring unknown option '-OT'
cl : Command line warning D9002 : ignoring unknown option '-O:'
cl : Command line warning D9002 : ignoring unknown option '-OR'
cl : Command line warning D9002 : ignoring unknown option '-OE'
cl : Command line warning D9002 : ignoring unknown option '-OF'
cl : Command line warning D9002 : ignoring unknown option '-OP'
cl : Command line warning D9002 : ignoring unknown option '-OT'
cl : Command line warning D9002 : ignoring unknown option '-O:'
cl : Command line warning D9002 : ignoring unknown option '-OI'
cl : Command line warning D9002 : ignoring unknown option '-OC'
cl : Command line warning D9002 : ignoring unknown option '-OF'
cl : Command line warning D9002 : ignoring unknown option '-warn'
cl : Command line warning D9024 : unrecognized source file type 'all', object file assumed

We should probably ignore -O3 and -warn as well. Honestly, I'm currently updating the in-tree libffi from upstream in bug 810631. Maybe we should just try to get these fixed there and get this for "free" that way.
Flags: needinfo?(khuey)
I don't know anything beyond what's in the bug.  I suspect the reason it never landed is that Mitch has been inactive for a few years.
Attached patch rebasedSplinter Review
Mike, does this look sensible to you? If so, my intent is to just get this upstreamed rather than landing it in-tree. We'll then pick it up in bug 810631.
Attachment #469552 - Attachment is obsolete: true
Attachment #491822 - Attachment is obsolete: true
Attachment #8384176 - Flags: review?(mh+mozilla)
Comment on attachment 8384176 [details] [diff] [review]
rebased

Review of attachment 8384176 [details] [diff] [review]:
-----------------------------------------------------------------

The biggest problem with this patch is that ltmain.sh comes from another third party, libtool, and I don't think they would accept this patch. Even if they would, you'd need to wait for another libtool release which then would then need to make it to another libffi release using it.
On top of that, the assumption in the libffi build system is that the given flags really are for gcc: the compiler we use for ffi is really msvcc.sh, which is a gcc-command-line-like wrapper around msvc. If there really needs to be (upstreamable) ldflags filtering, we should then have a similar wrapper around link.exe (or reuse msvcc.sh for that)
Attachment #8384176 - Flags: review?(mh+mozilla) → review-
Is this bug still actual, taking into account that VS2005 is no longer supported for building?
Flags: needinfo?(ryanvm)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(florian)
Whiteboard: [patchlove] → [patchlove][closeme 2017-05-15]
(In reply to Phoenix from comment #23)
> Is this bug still actual, taking into account that VS2005 is no longer
> supported for building?

Most likely not.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(florian)
Resolution: --- → INCOMPLETE
Flags: needinfo?(ryanvm)
Flags: needinfo?(mh+mozilla)
Whiteboard: [patchlove][closeme 2017-05-15] → [patchlove]
We're not using the libffi build system anyways, so this is now irrelevant.
Flags: needinfo?(jorendorff)
Sorry, I don't know where this needinfo came from.
Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.