Bug 204843 - Cross-compilation of Valgrind is broken (again)
Summary: Cross-compilation of Valgrind is broken (again)
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Developer tools
Component: general (other bugs)
Version First Reported In: 3.5.0
Platform: Compiled Sources Linux
: NOR normal
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-23 10:55 UTC by Bart Van Assche
Modified: 2009-08-30 22:45 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Bart Van Assche 2009-08-23 10:55:24 UTC
Version:           3.5.0 (using KDE 4.3.0)
Compiler:          gcc 4.1.1 gcc version 4.1.1
OS:                Linux
Installed from:    Compiled From Sources

Cross-compilation of Valgrind 3.5.0 fails during configure with the following error message:

$ cross-valgrind-build
[ ... ]
checking for TLS support... yes                                              
checking for /proc/self/fd... configure: error: cannot check for file existence when cross compiling
Comment 1 Bart Van Assche 2009-08-23 12:09:18 UTC
Fixed in r10860.
Comment 2 Bart Van Assche 2009-08-23 13:45:33 UTC
... together with r10863.
Comment 3 Julian Seward 2009-08-24 16:28:40 UTC
Can you explain a bit more about why it worked before and how it
got broken?  I see a bunch of #if HAVE_PROC that wasn't there before.
I'd prefer not to have #ifdef'd code unless we really need it.
Comment 4 Julian Seward 2009-08-24 16:31:12 UTC
In other words .. if it worked OK on 3.4.1 without the #ifdef'd code,
why do we need #ifdefs now?
Comment 5 Bart Van Assche 2009-08-24 19:17:49 UTC
(In reply to comment #4)
> In other words .. if it worked OK on 3.4.1 without the #ifdef'd code,
> why do we need #ifdefs now?

The addition of AC_CHECK_FILES() in r10156 broke cross-compilation. What I did is to remove the AC_CHECK_FILES() configure test and to replace #ifdef HAVE_PROC by VG_(have_proc_filesystem)().
Comment 6 Julian Seward 2009-08-25 15:44:12 UTC
(In reply to comment #5)
> The addition of AC_CHECK_FILES() in r10156 broke cross-compilation. What I did
> is to remove the AC_CHECK_FILES() configure test and to replace #ifdef
> HAVE_PROC by VG_(have_proc_filesystem)().

Hmm, I'm not happy about either the existence of AC_CHECK_FILES or the
fix.  What is the purpose of AC_CHECK_FILES in the first place?

My understanding of this is that

* on darwin, there is no /proc filesystem
* on linux, the target platform must have a /proc filesystem, and
  we don't care if the build platform has a /proc filesystem

Is that a correct analysis?
Comment 7 Bart Van Assche 2009-08-25 16:00:56 UTC
(In reply to comment #6)
> Hmm, I'm not happy about either the existence of AC_CHECK_FILES or the
> fix.  What is the purpose of AC_CHECK_FILES in the first place?
> 
> My understanding of this is that
> 
> * on darwin, there is no /proc filesystem
> * on linux, the target platform must have a /proc filesystem, and
>   we don't care if the build platform has a /proc filesystem
> 
> Is that a correct analysis?

How about embedded Linux systems where the procfs filesystem has not been mounted ? Do we support these ? If yes, then it has to be tested whether the currently implemented solution works on such a system. If we do not want to support Linux systems where procfs has not been mounted on /proc, then a check should be added during Valgrind startup that lets valgrind abort if VG_(have_proc_filesystem)() == 0.
Comment 8 Julian Seward 2009-08-25 16:32:10 UTC
> How about embedded Linux systems where the procfs filesystem has not been
> mounted ? Do we support these ?

No, we don't.  We need to read /proc/self/maps at startup in order to
find out the initial memory mapping.  If that isn't possible then the
run is aborted.

I believe the following would be sufficient:

* get rid of the configure test

* for Darwin, use existing code that is #if defined(VGO_darwin) to
  find out the initial memory map.  That uses some Mach magic and 
  doesn't make any use of /proc

* for Linux, using existing code that is #if defined(VGO_linux) to read
  /proc/self/maps.  If that can't be opened then we simply give up
  (which must have always been the behaviour on Linux anyway)

I am fully in support of having V cross compilable on Linux.  But I
think the introduction of the feature test is an unnecessary complication
which in turn has necessitates your fix, whereas in fact neither are
necessary.  (I would also be the first to say that I'm not 100% confident
I understand all the constraints yet :-)
Comment 9 Bart Van Assche 2009-08-25 16:55:57 UTC
(In reply to comment #8)
> I believe the following would be sufficient:
> 
> * get rid of the configure test
> 
> * for Darwin, use existing code that is #if defined(VGO_darwin) to
>   find out the initial memory map.  That uses some Mach magic and 
>   doesn't make any use of /proc
> 
> * for Linux, using existing code that is #if defined(VGO_linux) to read
>   /proc/self/maps.  If that can't be opened then we simply give up
>   (which must have always been the behaviour on Linux anyway)

How about reintroducing the HAVE_PROC preprocessor symbol and defining this symbol on Linux but not on Darwin ?
Comment 10 Julian Seward 2009-08-25 20:03:08 UTC
> How about reintroducing the HAVE_PROC preprocessor symbol and defining this
> symbol on Linux but not on Darwin ?

Why bother?  We already build aspacem and everything else with the relevant
VGO_os symbol defined; we can key everything needed from that.
Comment 11 Bart Van Assche 2009-08-25 20:17:56 UTC
(In reply to comment #10)
> > How about reintroducing the HAVE_PROC preprocessor symbol and defining this
> > symbol on Linux but not on Darwin ?
> 
> Why bother?  We already build aspacem and everything else with the relevant
> VGO_os symbol defined; we can key everything needed from that.

Inserting #ifdef VGO_linux everywhere #ifdef HAVE_PROC occurred would be a bad idea. This would not only make the source code less readable but also harder to port to new operating systems.
Comment 12 Bart Van Assche 2009-08-25 22:21:12 UTC
(In reply to comment #10)
> Why bother?  We already build aspacem and everything else with the relevant
> VGO_os symbol defined; we can key everything needed from that.

Implemented in r10867 and r10868. A more portable solution can be implemented later on. Note: the code touched by these revisions is not covered by any regression tests yet. I have verified proper operation of the touched code by verifying the output of the following command:
$ ./vg-in-place --trace-children=yes bash -c 'od -c /proc/self/cmdline; ls -l /proc/self/exe'
Comment 13 Bart Van Assche 2009-08-27 18:49:38 UTC
A regression test has been added via r10871 - r10873.
Comment 14 Julian Seward 2009-08-30 22:45:14 UTC
3_5 rev is 10877.