Last Comment Bug 631928 - Check for libffi_cv_as_x86_pcrel should not look for just "warning"
: Check for libffi_cv_as_x86_pcrel should not look for just "warning"
Status: RESOLVED FIXED
[fixed in tracemonkey]
:
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on:
Blocks: clang
  Show dependency treegraph
 
Reported: 2011-02-06 11:33 PST by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-05-06 14:40 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
don't check for 'warning' (2.10 KB, patch)
2011-03-22 07:09 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
ted: review+
Details | Diff | Splinter Review
patch including js/src/ctypes/libffi.patch (13.60 KB, patch)
2011-04-14 13:48 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
no flags Details | Diff | Splinter Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-02-06 11:33:58 PST
I posted the patch upstream:

http://sourceware.org/ml/libffi-discuss/2011/msg00024.html

but not reply so far.

The problem is that the configure for ffi does two things which are strange at least

* uses CFLAGS when processing an assembly file
* greps the output instead of checking the error code

This makes it fail when trying to build with lto since clang will produce an warning about '-emit-llvm' not being used (since it is processing an assembly file).

The above patch changes configure to use the error code. Dropping CFLAGS would also solve it.
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-03-22 07:09:02 PDT
Created attachment 520911 [details] [diff] [review]
don't check for 'warning'
Comment 2 Ted Mielczarek [:ted.mielczarek] 2011-03-24 11:11:11 PDT
Comment on attachment 520911 [details] [diff] [review]
don't check for 'warning'

This seems sensible enough, but I'd really rather have someone from upstream chime in. (Also, we do need to push all libffi changes upstream for sanity's sake.)
Comment 3 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-03-26 20:56:15 PDT
I have pinged the upstream thread:
http://www.cygwin.com/ml/libffi-discuss/2011/msg00110.html
Comment 4 Lars Sonchocky-Helldorf 2011-04-09 09:19:39 PDT
@Rafael:

Maybe the topic of your mail to libffi-discuss was to cryptic so nobody knows what you envision to do with this patch. I know, the explanation is in your mail but some people don't have the time to read every mail with the necessary attentiveness. And then again I came across some developer who didn't know what llvm was. So I would say repost your mail with a more descriptive topic which lays out your intention clearly.

Btw. I am not affiliated with the libffi-project, I just want to solve some communication problems.
Comment 5 dwitte@gmail.com 2011-04-09 09:22:17 PDT
The libffi list isn't always immediately responsive, either. You might have to ping the list a few times to get a response.
Comment 6 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-10 09:33:16 PDT
Trying yet again:

http://www.cygwin.com/ml/libffi-discuss/2011/msg00116.html

Do we have a strong policy of never having local patches to libffi? Once NSS merges to m-c this will be one of the two bugs (the other being 635790) blocking building m-c with clang.
Comment 7 dwitte@gmail.com 2011-04-14 11:05:35 PDT
We do have local patches to libffi, but we try to keep them minimal. Since you've pushed thrice to upstream, we can take it. :)

Can you roll a new patch with your additions added to js/src/ctypes/libffi.patch, with the bug# referenced?
Comment 8 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-14 13:48:06 PDT
Created attachment 526097 [details] [diff] [review]
patch including js/src/ctypes/libffi.patch

I am not sure how js/src/ctypes/libffi.patch was generated the first time. To try to reduce the noise (file order for example) in this change I did

echo > js/src/ctypes/libffi.patch
for i in js/src/ctypes/libffi/Makefile.in js/src/ctypes/libffi/configure   js/src/ctypes/libffi/src/x86/ffi64.c js/src/ctypes/libffi/src/arm/ffi.c js/src/ctypes/libffi/src/arm/ffitarget.h js/src/ctypes/libffi/src/arm/sysv.S js/src/ctypes/libffi/testsuite/lib/libffi-dg.exp js/src/ctypes/libffi/testsuite/libffi.call/cls_double_va.c js/src/ctypes/libffi/testsuite/libffi.call/cls_longdouble_va.c js/src/ctypes/libffi/include/ffi.h.in js/src/ctypes/libffi/configure.ac; do git diff -U8  tmp tmp2 $i >> js/src/ctypes/libffi.patch; done
Comment 9 dwitte@gmail.com 2011-04-21 13:11:53 PDT
Sounds fine, as long as everything's still in there!
Comment 10 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-04-21 13:53:32 PDT
To the best of my knowledge it is.
Comment 11 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-05-06 14:40:13 PDT
This was fixed some time ago.

Note You need to log in before you can comment on or make changes to this bug.