Hi Jonathan,
Let's consider e.g.
src/share/native/sun/awt/splashscreen/java_awt_SplashScreen.c mentioned
by you below.
It #includes splashscreen_impl.h. This file #includes
splashscreen_config.h. And finally,
src/solaris/native/sun/awt/splashscreen/splashscreen_config.h #includes
the stdlib.h explicitly.
So, do you still see any issues with java_awt_SplashScreen.c?
Could you please go through your list and verify carefully whether these
files actually #include stdlib.h or not?
--
best regards,
Anthony
On 2/3/2012 12:47 PM, Jonathan Lu wrote:
Sorry for being absent from this thread so long, I just experienced
couple of days with very limited Internet access.
Actually the problem was firstly found on AIX platform, on which
sizeof(malloc(1)) without including <stdlib.h> will always give 4
instead of 8. The symptom was runtime segment error not compiling time
warnings, and I suspect there're more failures which were not covered in
my application.
I've checked the build log of Linux64 platform, as Phil mentioned
there's also no such warnings except for the hotspot diagnostic ones.
One reason might be that some code inlcudes "malloc.h" instead of
<stdlib.h>, there's one 'extern' definition in malloc.h on Linux 64
platform, but I do not see one on AIX platform. I've also tried to trace
the indirect reference relation ship for all the .c/.cpp files using
malloc(), still cannot determine a explicit definition of following
files. pls fix me if anything wrong, because the list is produced
manually :(
./src/share/native/sun/awt/splashscreen/splashscreen_png.c
./src/share/native/sun/awt/splashscreen/java_awt_SplashScreen.c
./src/share/native/sun/awt/splashscreen/splashscreen_gif.c
./src/share/native/sun/awt/splashscreen/splashscreen_impl.c
./src/share/native/sun/awt/image/dither.c
./src/share/native/sun/font/layout/KernTable.cpp
./src/share/native/sun/java2d/cmm/lcms/cmserr.c
./src/share/native/com/sun/media/sound/PlatformMidi.c
./src/share/bin/jli_util.c
./src/share/back/debugInit.c
./src/share/demo/jvmti/hprof/hprof_init.c
./src/share/demo/jvmti/hprof/hprof_table.c
./src/share/demo/jvmti/hprof/hprof_util.c
./src/windows/native/java/lang/ProcessImpl_md.c
./src/windows/native/java/lang/java_props_md.c
./src/windows/native/java/net/DualStackPlainDatagramSocketImpl.c
./src/windows/native/java/io/dirent_md.c
./src/windows/native/java/io/io_util_md.c
./src/windows/native/sun/awt/splashscreen/splashscreen_sys.c
./src/windows/native/sun/tracing/dtrace/jvm_symbols_md.c
./src/windows/native/sun/windows/awt_new.cpp
./src/windows/native/sun/windows/CmdIDList.cpp
./src/windows/native/sun/nio/ch/DatagramDispatcher.c
./src/windows/native/sun/nio/ch/SocketDispatcher.c
./src/windows/native/com/sun/media/sound/PLATFORM_API_WinOS_MidiIn.cpp
./src/windows/native/com/sun/media/sound/PLATFORM_API_WinOS_Ports.c
./src/windows/native/com/sun/media/sound/PLATFORM_API_WinOS_DirectSound.cpp
./src/windows/native/com/sun/media/sound/PLATFORM_API_WinOS_Util.c
./src/windows/native/com/sun/media/sound/PLATFORM_API_WinOS_MidiOut.c
./src/solaris/native/sun/java2d/opengl/GLXSurfaceData.c
./src/solaris/native/sun/java2d/x11/XRBackendNative.c
./src/solaris/native/sun/java2d/x11/X11SurfaceData.c
./src/solaris/native/sun/java2d/x11/X11TextRenderer_md.c
./src/solaris/native/sun/java2d/x11/X11Renderer.c
For a simple HelloWorld.c, if we use malloc() without including
stdlib.h, it will give warnings like "warning: incompatible implicit
declaration of built-in function ‘malloc’". No such warning were found
from the build log for above files, so I guess there must be some
headers has got indirect including or declaration of malloc.h, but I'm
worried because I did not see one via manual search. It indeed sounds
reasonable to me that if no warnings are produced and the compilation
runs OK, we may just leave the code untouched, but it will improve
portability if we can explicitly includes standard C library headers in
the right place (of cause, the 'right' place is also something need to
be discussed), right?
And for the 3rd party libraries, for those whose code has been shipped
with OpenJDK source, they seem OK to me since malloc() has either been
included or redefined. But for some dependency libraries of compilation,
such as X11 dev, are they also trustworthy?
Best regards!
- Jonathan
On 01/19/2012 02:31 AM, Kelly O'Hair wrote:
A webrev and a code review from the appropriate teams is indeed needed.
-kto
On Jan 18, 2012, at 9:56 AM, Phil Race wrote:
Its arguable, whether harmless or not, that each file needs to
include it.
Its not unreasonable for an area of the code to have a header file
such as "awt.h"
that is supposed to be the one that's included by the other files in
that area of
the code, and which takes care of common includes. jni_util.h is not
necessarily it.
There isn't a need for every file to include that.
Also many files are 3rd party libs and I don't like editing those as
the changes
really belong upstream.
So a one size fits all approach might be the answer but I'd want to
make sure
of that first.
So I'd like to see the list of files that generate actual warnings as
well as the list
of files that reference malloc but don't include stdlib.h.
We are well aware that returning int as a default is bad in 64 bit ..
I'd expect
instant death so I'd like to see those actual warnings rather than
just the
theoretical ones.
My grep of a current JDK 8 build log for 64 bit Linux shows the only
malloc warnings
are in hotspot management code. So I am waiting for the proof of the
real problem
And I can speak for 2d, and if there's 2D files touched I would like
to see any changes
to those files, and the reasoning discussed on 2d-dev ..
-phil.
On 1/18/2012 8:26 AM, Kelly O'Hair wrote:
On Jan 18, 2012, at 12:19 AM, Jonathan Lu wrote:
Hi core-libs-dev,
I found that for some native code of OpenJDK code base, malloc() is
used without including header file stdlib.h, such as following files,
./src/solaris/native/sun/java2d/opengl/GLXSurfaceData.c
./src/solaris/native/sun/java2d/x11/XRBackendNative.c
....
I assume that there's no hacking tricks involved here, right?
because this may cause problem for some C compilers, which assumes
'int' as the default return type of a function if it cannot find
the function's declaration during compiling. Under such a
condition, actual return result of type 'void*' from malloc() will
be converted to 'int', which may result in truncated pointers in
64bit platforms. If the application tries to dereference such a
broken pointer, error will occur.
Indeed I found some indirect includes of stdlib.h, but there're
still some I do not see a stdlib.h get included from any of the
direct/indirect included headers. I think in order to fix this
problem, two approaches may be considered here,
a) add "#include<stdlib.h>" to every missing .c file.
b) add "#include<stdlib.h>" to a commonly referenced header file,
such as jni_util.h. but it would not be easy to find such a file
for all and creating one is the same as approach a).
I suggest a) It should be harmless and is the right thing to do.
It's been a while since I studied the C standard, but I vaguely
recall that there was another standard C include file
that would cause the malloc() prototype declaration to show up.
Or maybe it wasn't a standard one. In any case, I think your a)
approach is correct and I don't see much a need
for detailed discussions on this, as long as it builds correctly
with no warnings on all platforms. It should be fine and correct.
That's my 2 cents.
-kto
But both methods need to change many files, any other ideas about
how to fix it more elegantly?
Thanks and best regards!
- Jonathan
