Hi Goetz,
This has been pushed but there is something odd with the changeset
timestamp:
date Thu, 17 Aug 2017 17:26:02 +0200 (11 days ago)
???
David
On 29/08/2017 8:14 PM, David Holmes wrote:
> On 29/08/2017 8:11 PM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> Thomas, thanks for looking at the new webrev ad hoc.
>>
>>>> 2) If the user provided buffer is too small, we will fail, which looks
>>>> like the dll could not have been located. I am not sure we have to be
>>>> shy with allocating memory - internally, we malloc a buffer for
>>>> assembling the filename, and then os::splitpath() will malloc a whole
>>>> bunch of arrays too. So I think we could just return the dll path
>>>> location in a malloced buffer and require the caller to free.
>>>
>>> That seems a much bigger change. My question before pushing this is:
>>> have we in any way reduced the size of path that we may accept on some
>>> platforms? If we have that would be bad.
>> No. I nowhere change the size of the buffer passed into this method,
>> and that's the limit.
>
> Thanks for confirming - I will push this now.
>
> David
> -----
>
>> On posix, paths no more contain // where concatenated, so the
>> paths supported can contain one more char in this case. This is an
>> improvement.
>> The buffer passed in usually has size MAX_PATH, so the checks for
>> overflow should never cause a problem. They are mostly there
>> for safety. Allocation of the memory rather would reduce memory
>> consumption, but I think it's not that relevant. Overall, I think it's
>> a mismatch that some functions allocate the memory, others
>> require a buffer ... but that's really out of scope.
>>
>> Best regards,
>> Goetz.
>>
>>
>>
>>
>>>
>>>> 3) I do not understand ':' as a file separator on windows. So, a
>>>> path is
>>>> allowed to contain e.g. "C:;" ? Which would mean "a path relative to
>>>> the
>>>> current directory currently active on drive C". If I am not
>>>> mistaken. Do
>>>> we want to support this, what is the use case?
>>>
>>> IIUC the existing windows code supports this. So yes c:foo.dll is a
>>> reference to foo.dll in whatever the current directory on drive c: is.
>>> As for a usecase ... perhaps a way to workaround long paths or
>>> historical path format restrictions (ie no spaces) ? But the main thing
>>> is to not break what we currently support.
>>>
>>> Thanks,
>>> David
>>>
>>>> Kind Regards, Thomas
>>>>
>>>>
>>>> On Tue, Aug 29, 2017 at 10:41 AM, David Holmes
>>> <david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>
>>>> On 29/08/2017 6:29 PM, Lindenmaier, Goetz wrote:
>>>>
>>>> Hi David,
>>>>
>>>> I fixed the indentation and added you as reviewer.
>>>> I replaced the webrev in-place:
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>> dllBuildName/webrev.05/
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>> dllBuildName/webrev.05/>
>>>> The new code went through all our testing ... except for some
>>>> ppc/s390 builds
>>>> that failed because of an other change pushed to hs
>>>> tonight. But
>>>> that should
>>>> not matter, it all passed with the jdk10/jdk10 testing.
>>>>
>>>> Would you mind sponsoring?
>>>>
>>>>
>>>> No problem - but can we get Thomas to sign-off on this latest
>>>> version please.
>>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>> -----Original Message-----
>>>> From: David Holmes [mailto:david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>]
>>>> Sent: Dienstag, 29. August 2017 09:53
>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>>
>>>> Cc: hotspot-runtime-dev at openjdk.java.net
>>>> <mailto:hotspot-runtime-dev at openjdk.java.net>
>>>> Subject: Re: [ping] RFR(M): 8186072: dll_build_name
>>>> returns
>>>> true even if file
>>>> is missing.
>>>>
>>>> Hi Goetz,
>>>>
>>>> On 29/08/2017 4:18 PM, Lindenmaier, Goetz wrote:
>>>>
>>>> Hi,
>>>>
>>>> this is a webrev with merged windows and posix
>>>> implementations:
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> dllBuildName/webrev.05/
>>>>
>>>> I like the look of this.
>>>>
>>>> There are a couple of indention nits in os.cpp:
>>>>
>>>> 247 static bool conc_path_file_and_check(char
>>>> *buffer, char
>>>> *printbuffer, size_t printbuflen,
>>>> 248 const char* pname,
>>>> char lastchar,
>>>> const char* fname) {
>>>>
>>>>
>>>> 251 const char *filesep = (WINDOWS_ONLY(lastchar ==
>>>> ':' ||) lastchar
>>>> == os::file_separator()[0]) ?
>>>> 252 "" : os::file_separator();
>>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> Best regards,
>>>> Goetz
>>>>
>>>> -----Original Message-----
>>>> From: Lindenmaier, Goetz
>>>> Sent: Montag, 28. August 2017 12:10
>>>> To: 'David Holmes' <david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>>
>>>> Cc: hotspot-runtime-dev at openjdk.java.net
>>>> <mailto:hotspot-runtime-dev at openjdk.java.net>
>>>> Subject: RE: [ping] RFR(M): 8186072:
>>>> dll_build_name
>>>> returns true even if
>>>>
>>>> file
>>>>
>>>> is missing.
>>>>
>>>> Hi,
>>>>
>>>> this are the changes needed to make the windows
>>>> dll_locate_lib
>>>> universally applicable. I also merge the three
>>>> similar jio_snprintf
>>>> calls into one method.
>>>> I do some gymnastics to avoid another buffer of
>>>> MAX_PATH_LEN
>>>> at the first call to conc_path_file_and_check.
>>>> I'll test this tonight.
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>> diff -r e09e4eb985c5
>>>> src/os/windows/vm/os_windows.cpp
>>>> --- a/src/os/windows/vm/os_windows.cpp Thu Aug 17
>>>> 17:26:02
>>>>
>>>> 2017
>>>>
>>>> +0200
>>>> +++ b/src/os/windows/vm/os_windows.cpp Mon Aug 28
>>>> 12:02:26
>>>>
>>>> 2017
>>>>
>>>> +0200
>>>> @@ -1205,6 +1205,17 @@
>>>> return GetFileAttributes(filename) !=
>>>> INVALID_FILE_ATTRIBUTES;
>>>> }
>>>>
>>>> +bool conc_path_file_and_check(char *buffer, char
>>>> *printbuffer, size_t
>>>> printbuflen,
>>>> + const char* pname,
>>>> char lastchar, const char* fname) {
>>>> + char *filesep = (WINDOWS_ONLY(lastchar ==
>>>> ':' ||)
>>>> lastchar ==
>>>> os::file_seperator()[0]) ? "" :
>>>> os::file_separator();
>>>> + int ret = jio_snprintf(printbuffer,
>>>> printbuflen,
>>>> "%s%s%s", path, filesep,
>>>> fullfname);
>>>> + if (ret != -1) {
>>>> + struct stat statbuf;
>>>> + return os::stat(buffer, &statbuf) == 0;
>>>> + }
>>>> + return false;
>>>> +}
>>>> +
>>>> bool os::dll_locate_lib(char *buffer,
>>>> size_t buflen,
>>>> const char* pname,
>>>> const
>>>> char* fname) {
>>>> bool retval = false;
>>>> @@ -1220,11 +1231,8 @@
>>>> if (p != NULL) {
>>>> const size_t plen = strlen(buffer);
>>>> const char lastchar = buffer[plen -
>>>> 1];
>>>> - char *filesep = (lastchar == ':' ||
>>>> lastchar == '\\') ? "" : "\\";
>>>> - int ret = jio_snprintf(&buffer[plen],
>>>> buflen - plen, "%s%s", filesep,
>>>> fullfname);
>>>> - if (ret != -1) {
>>>> - retval = file_exists(buffer);
>>>> - }
>>>> + retval = conc_path_file_and_check(buffer,
>>>> &buffer[plen], buflen -
>>>> plen,
>>>> + "",
>>>> lastchar, fullfname);
>>>> }
>>>> } else if (strchr(pname,
>>>> *os::path_separator()) != NULL) {
>>>> int n;
>>>> @@ -1238,12 +1246,8 @@
>>>> continue; // skip the empty
>>>> path values
>>>> }
>>>> const char lastchar = path[plen -
>>>> 1];
>>>> - char *filesep = (lastchar == ':' ||
>>>> lastchar == '\\') ? "" : "\\";
>>>> - int ret = jio_snprintf(buffer, buflen,
>>>> "%s%s%s", path, filesep,
>>>> fullfname);
>>>> - if (ret != -1 && file_exists(buffer)) {
>>>> - retval = true;
>>>> - break;
>>>> - }
>>>> + retval =
>>>> conc_path_file_and_check(buffer,
>>>> buffer, buflen, path,
>>>> lastchar, fullfname);
>>>> + if (retval) break;
>>>> }
>>>> // release the storage
>>>> for (int i = 0; i < n; i++) {
>>>> @@ -1255,11 +1259,7 @@
>>>> }
>>>> } else {
>>>> const char lastchar = pname[pnamelen-1];
>>>> - char *filesep = (lastchar == ':' ||
>>>> lastchar
>>>> == '\\') ? "" : "\\";
>>>> - int ret = jio_snprintf(buffer, buflen,
>>>> "%s%s%s", pname, filesep,
>>>> fullfname);
>>>> - if (ret != -1) {
>>>> - retval = file_exists(buffer);
>>>> - }
>>>> + retval = conc_path_file_and_check(buffer,
>>>> buffer, buflen, path,
>>>>
>>>> lastchar,
>>>>
>>>> fullfname);
>>>> }
>>>> }
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: David Holmes
>>>> [mailto:david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>]
>>>> Sent: Montag, 28. August 2017 07:38
>>>> To: Lindenmaier, Goetz
>>>> <goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>>
>>>> Cc: hotspot-runtime-dev at openjdk.java.net
>>>> <mailto:hotspot-runtime-dev at openjdk.java.net>
>>>> Subject: Re: [ping] RFR(M): 8186072:
>>>> dll_build_name returns true even if
>>>>
>>>> file
>>>>
>>>> is missing.
>>>>
>>>> Hi Goetz,
>>>>
>>>> On 25/08/2017 12:19 AM, Lindenmaier, Goetz
>>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> I please need a second review and a
>>>> sponsor:
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> dllBuildName/webrev.04
>>>>
>>>>
>>>> To update my description of the change to
>>>> the status after Thomas'
>>>>
>>>> review:
>>>>
>>>>
>>>> dll_build_name builds the proper path to a
>>>> library given a list of paths
>>>>
>>>> separated by
>>>>
>>>> path_seperator and a library name. It adds
>>>> in the platform specific
>>>>
>>>> endings
>>>>
>>>> etc.
>>>>
>>>> It is documented to return whether the
>>>> file
>>>> exists, but only does so if a
>>>>
>>>> path_seperator
>>>>
>>>> exists in the path.
>>>> Especially if the path is empty, it just
>>>> returns ‘true’ without checking.
>>>>
>>>> Dll_build_name is usually used before
>>>> calling dll_load. If dll_load does
>>>>
>>>> not
>>>>
>>>> get a full path it searches
>>>>
>>>> in well known unix/windows locations. This
>>>> is intended in the two cases
>>>>
>>>> where dll_build_name
>>>>
>>>> is called with an empty path.
>>>>
>>>> I renamed dll_build_name to dll_locate_lib
>>>> and changed it's behavior to
>>>>
>>>> always return
>>>>
>>>> a full path to the lib, inserting current
>>>> working directory if no path is
>>>>
>>>> given.
>>>>
>>>> For the use case where "" was actually
>>>> passed to the function, I added
>>>>
>>>> a
>>>>
>>>> new function
>>>>
>>>> (reusing the old function name)
>>>> dll_build_name that just adds system
>>>>
>>>> dependent prefix and suffix
>>>>
>>>> to the name.
>>>> I merged all unix implementations to the
>>>> posix os branch.
>>>>
>>>>
>>>> I started to look at this and have applied the
>>>> patch to run through some
>>>> basic testing. The overall approach seems
>>>> reasonable. But it is hard to
>>>> track all the details - in particular whether
>>>> there were any subtle
>>>> differences across the "posix" systems?
>>>>
>>>> I'm wondering what, if any, significant
>>>> differences exist between the
>>>> Windows and POSIX versions? I would hope the
>>>> platform differences
>>>>
>>>> could
>>>>
>>>> easily be hidden behind macros (for path
>>>> separator, library suffix etc).
>>>> Then perhaps this could just go in shared code
>>>> (os.hpp, os.cpp)?
>>>>
>>>> That aside, in the Windows code shouldn't the
>>>> hardwired .dll strings
>>>> actually be JNI_LIB_SUFFIX?
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Thomas Stüfe
>>>> [mailto:thomas.stuefe at gmail.com
>>>> <mailto:thomas.stuefe at gmail.com>]
>>>> Sent: Dienstag, 22. August 2017 17:30
>>>> To: Lindenmaier, Goetz
>>>> <goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>>
>>>> Cc:
>>>> hotspot-runtime-dev at openjdk.java.net
>>>>
>>>> <mailto:hotspot-runtime-dev at openjdk.java.net>
>>>> Subject: Re: RFR(M): 8186072:
>>>> dll_build_name returns true even if
>>>> file
>>>>
>>>> is
>>>>
>>>> missing.
>>>>
>>>> Looks good.
>>>>
>>>> ..Thomas
>>>>
>>>> On Tue, Aug 22, 2017 at 4:33 PM,
>>>> Lindenmaier, Goetz
>>>> <goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>
>>>> <mailto:goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>>
>>>>
>>>>
>>>> wrote:
>>>>
>>>>
>>>>
>>>> I mistyped the path to
>>>> webrev,
>>>> this should work:
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>> dllBuildName/webrev.04
>>>>
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> dllBuildName/webrev.04>
>>>>
>>>> Sorry,
>>>> Goetz
>>>>
>>>>
>>>>
>>>> > -----Original Message-----
>>>> > From: Lindenmaier, Goetz
>>>> > Sent: Dienstag, 22. August
>>>> 2017 15:48
>>>> > To: 'Thomas Stüfe'
>>>> <thomas.stuefe at gmail.com
>>>> <mailto:thomas.stuefe at gmail.com>
>>>> <mailto:thomas.stuefe at gmail.com
>>>> <mailto:thomas.stuefe at gmail.com>> >
>>>> > Cc:
>>>> hotspot-runtime-dev at openjdk.java.net
>>>>
>>>> <mailto:hotspot-runtime-dev at openjdk.java.net>
>>>> <mailto:hotspot- <mailto:hotspot->
>>>>
>>>> runtime-
>>>>
>>>> dev at openjdk.java.net
>>>> <mailto:dev at openjdk.java.net>>
>>>> > Subject: RE: RFR(M):
>>>> 8186072:
>>>> dll_build_name returns true even if
>>>> file is
>>>> > missing.
>>>> >
>>>> > Hi,
>>>> >
>>>> > could I please get a second
>>>> review?
>>>> >
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>> dllBuildName-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>> dllBuildName->
>>>> hs/webrev.04
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> dllBuildName-
>>>>
>>>> hs/webrev.04>
>>>> >
>>>> > I had to update the webrev
>>>> because of a problem on windows.
>>>> > @Thomas I had edited
>>>> os.hpp,
>>>> but not saved :(
>>>> >
>>>> > Best regards,
>>>> > Goetz.
>>>> >
>>>> > PS: Didn't double-check the
>>>> webrev as cr server is slow.
>>>> >
>>>> > > -----Original
>>>> Message-----
>>>> > > From: Thomas Stüfe
>>>> [mailto:thomas.stuefe at gmail.com
>>>> <mailto:thomas.stuefe at gmail.com>
>>>> <mailto:thomas.stuefe at gmail.com
>>>> <mailto:thomas.stuefe at gmail.com>> ]
>>>> > > Sent: Donnerstag, 17.
>>>> August 2017 19:54
>>>> > > To: Lindenmaier, Goetz
>>>> <goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>
>>>> <mailto:goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>> >
>>>> > > Cc:
>>>> hotspot-runtime-dev at openjdk.java.net
>>>>
>>>> <mailto:hotspot-runtime-dev at openjdk.java.net>
>>>> <mailto:hotspot- <mailto:hotspot->
>>>> runtime-dev at openjdk.java.net
>>>> <mailto:runtime-dev at openjdk.java.net>>
>>>> > > Subject: Re: RFR(M):
>>>> 8186072: dll_build_name returns
>>>> true even
>>>>
>>>> if
>>>>
>>>> file is
>>>> > > missing.
>>>> > >
>>>> > > Hi Goetz,
>>>> > >
>>>> > > On Thu, Aug 17, 2017 at
>>>> 6:03 PM, Lindenmaier, Goetz
>>>> > >
>>>> <goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>
>>>> <mailto:goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>>
>>>>
>>>> <mailto:goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>
>>>>
>>>> <mailto:goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>>
>>>> > >
>>>> wrote:
>>>> > >
>>>> > >
>>>> > > Hi Thomas,
>>>> > >
>>>> > >
>>>> > >
>>>> > > I adapted the
>>>> comments
>>>> in os.hpp.
>>>> > >
>>>> > >
>>>> > >
>>>> > > If I move the call to
>>>> dll_build_name out of dll_locate_lib
>>>> > >
>>>> > > I have to do a lot of
>>>> coding in all the places where it
>>>> is called.
>>>> > >
>>>> > > That seems not useful
>>>> to me.
>>>> > >
>>>> > >
>>>> > >
>>>> > > Fixed the type to
>>>> size_t.
>>>> > >
>>>> > >
>>>> > >
>>>> > > One could merge
>>>> posix/windows if putting the check
>>>> for ‘:’
>>>> > >
>>>> > > into a
>>>> WINDOWS_ONLY() I
>>>> guess. The check for \ could be
>>>> > >
>>>> > > done in posix as
>>>> well,
>>>> if using file_seperator().
>>>> > >
>>>> > >
>>>> > >
>>>> > > * Not your change,
>>>> but: why does the code in
>>>>
>>>> os::dll_locate_lib()
>>>>
>>>> even
>>>> > >
>>>> > > * differentiate
>>>> between a PATH containing no
>>>> os::path_separator()
>>>> > >
>>>> > > * and a path
>>>> containing os::path_separator()?
>>>> > >
>>>> > > I assume this was
>>>> done
>>>> to avoid all the allocations and
>>>> copying
>>>>
>>>> of
>>>>
>>>> the
>>>> > > path.
>>>> > >
>>>> > >
>>>> > >
>>>> > > Also adapted the
>>>> comment in jvmtiExport.cpp.
>>>> > >
>>>> > >
>>>> > >
>>>> > > New webrev:
>>>> > >
>>>> > >
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > > dllBuildName/webrev.03/
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > > dllBuildName/webrev.03/>
>>>> > >
>>>> > > incremental diff:
>>>> > >
>>>> > >
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>> dllBuildName/webrev.03/diffs-incremental.patch
>>>> > >
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>> dllBuildName/webrev.03/diffs-incremental.patch>
>>>> > >
>>>> > > (fixed indentation on
>>>> windows)
>>>> > >
>>>> > >
>>>> > >
>>>> > > Best regards,
>>>> > >
>>>> > > Goetz.
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > > Comments in os.hpp seem
>>>> unchanged ?
>>>> > >
>>>> > > But looks fine
>>>> otherwise. I
>>>> do not need another webrev.
>>>> > >
>>>> > > Thanks, Thomas
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > > From: Thomas Stüfe
>>>> [mailto:thomas.stuefe at gmail.com
>>>> <mailto:thomas.stuefe at gmail.com>
>>>> <mailto:thomas.stuefe at gmail.com
>>>> <mailto:thomas.stuefe at gmail.com>>
>>>> > >
>>>> <mailto:thomas.stuefe at gmail.com
>>>> <mailto:thomas.stuefe at gmail.com>
>>>> <mailto:thomas.stuefe at gmail.com
>>>> <mailto:thomas.stuefe at gmail.com>> > ]
>>>> > > Sent: Thursday,
>>>> August
>>>> 17, 2017 3:48 PM
>>>> > > To: Lindenmaier,
>>>> Goetz
>>>> <goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>
>>>> <mailto:goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>>
>>>> > >
>>>> <mailto:goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>
>>>> <mailto:goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>>
>>>> > >
>>>> > > Cc:
>>>> hotspot-runtime-dev at openjdk.java.net
>>>>
>>>> <mailto:hotspot-runtime-dev at openjdk.java.net>
>>>> <mailto:hotspot- <mailto:hotspot->
>>>> runtime-dev at openjdk.java.net
>>>> <mailto:runtime-dev at openjdk.java.net>>
>>>> <mailto:hotspot-runtime-
>>>> <mailto:hotspot-runtime->
>>>>
>>>> <mailto:hotspot- <mailto:hotspot->
>>>>
>>>> runtime->
>>>> > > dev at openjdk.java.net
>>>> <mailto:dev at openjdk.java.net>
>>>> <mailto:dev at openjdk.java.net
>>>> <mailto:dev at openjdk.java.net>> >
>>>> > > Subject: Re: RFR(M):
>>>> 8186072: dll_build_name returns true
>>>>
>>>> even
>>>>
>>>> if file
>>>> > > is missing.
>>>> > >
>>>> > >
>>>> > >
>>>> > > Hi Goetz,
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > > On Thu, Aug 17,
>>>> 2017 at
>>>> 1:35 PM, Lindenmaier, Goetz
>>>> > >
>>>> <goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>
>>>> <mailto:goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>>
>>>>
>>>> <mailto:goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>
>>>>
>>>> <mailto:goetz.lindenmaier at sap.com
>>>> <mailto:goetz.lindenmaier at sap.com>>
>>>> > >
>>>> wrote:
>>>> > >
>>>> > > Hi Thomas,
>>>> > >
>>>> > > I reworked
>>>> the
>>>> whole thing.
>>>> > >
>>>> > > First,
>>>> there is
>>>> dll_build_name. It just does <name> ->
>>>> > > lib<name>.so.
>>>> > >
>>>> > > Second, I
>>>> renamed the legacy dll_build_name to
>>>> dll_locate_lib.
>>>> > >
>>>> > > I merged all
>>>> the unix variants to one in os_posix.
>>>> > >
>>>> > > I removed the
>>>> buffer overflow check at the top.
>>>> > > It's too
>>>> restrictive because the path argument
>>>> > > can contain
>>>> several paths. I added the overflow
>>>> > > checks
>>>> into the
>>>> single cases.
>>>> > >
>>>> > > Also, I first
>>>> assemble the pure name using the
>>>> new, simple
>>>> > >
>>>> dll_build_name.
>>>> This is for reuse and readability.
>>>> > >
>>>> > > In case of an
>>>> empty directory, I use
>>>> get_current_directory
>>>> > > to
>>>> complete the
>>>> path as indicated by the original
>>>> > > documentation
>>>> > > where it was
>>>> called with "".
>>>> > >
>>>> Dll_locate_lib
>>>> now always returns a name with a full
>>>> path if
>>>> > > the file
>>>> exists.
>>>> > >
>>>> > > Also, on
>>>> windows, I think I fixed a bug by
>>>> reversing the
>>>>
>>>> order
>>>>
>>>> > > of checks. A
>>>> path list ending in ':' or '\'
>>>> would not
>>>> have
>>>> > > been
>>>> recognized.
>>>> > >
>>>> > > On Bsd, I
>>>> removed JNI_LIB_* because that
>>>> already is
>>>>
>>>> defined
>>>>
>>>> > > in jvm_bsh.h
>>>> > >
>>>> > > New webrev:
>>>> > >
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > > dllBuildName/webrev.02/
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > > dllBuildName/webrev.02/>
>>>> > >
>>>> > > Best regards,
>>>> > > Goetz.
>>>> > >
>>>> > >
>>>> > >
>>>> > > I like this better
>>>> than
>>>> before. Remarks:
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>>
>>> dllBuildName/webrev.02/src/share/vm/runtime/os.hpp.udiff.html
>>>>
>>>> > >
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>>
>>> dllBuildName/webrev.02/src/share/vm/runtime/os.hpp.udiff.html>
>>>>
>>>> > >
>>>> > >
>>>> > >
>>>> > > + // Builds the
>>>> platform-specific name of a library.
>>>> > >
>>>> > > + // Returns
>>>> false on
>>>> __buffer overflow__.
>>>> > >
>>>> > >
>>>> > >
>>>> > > Hopefully not! :D
>>>> > >
>>>> > > How about: "Returns
>>>> false no truncation" instead.
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > > + // Builds a
>>>> platform-specific full library path
>>>> given an ld path
>>>> and lib
>>>> > > name.
>>>> > >
>>>> > > + // Returns true if
>>>> the buffer contains a full path to an
>>>> existing
>>>> file,
>>>> > > false
>>>> > >
>>>> > > + // otherwise. If
>>>> pathname is empty, checks the current
>>>> directory.
>>>> > >
>>>> > > + static bool
>>>> dll_locate_lib(char* buffer,
>>>> size_t size,
>>>> > >
>>>> > >
>>>> const char*
>>>> pathname,
>>>> const char*
>>>> fname);
>>>> > >
>>>> > >
>>>> > >
>>>> > > Might be worth
>>>> mentioning that "fname" is the
>>>> unadorned
>>>>
>>>> library
>>>>
>>>> > > name, e.g. "verify" for
>>>> libverify.so or verify.dll.
>>>> > >
>>>> > >
>>>> > >
>>>> > > Would the following
>>>> alternative be valid:
>>>> > >
>>>> > >
>>>> > >
>>>> > > one could make
>>>> dll_locate_lib take the real file
>>>> name,
>>>> and let
>>>> caller
>>>> > > use dll_build_name() to
>>>> build the libary name first before
>>>> handing
>>>>
>>>> it
>>>>
>>>> to
>>>> > > dll_locate_lib(). In that
>>>> case, dll_locate_lib() could be
>>>> renamed to
>>>>
>>>> a
>>>>
>>>> generic
>>>> > > "find_file_in_path"
>>>> because
>>>> it would work for any kind of file.
>>>> > >
>>>> > >
>>>> > >
>>>> > > As an added bonus,
>>>> there would be no need to create a
>>>> temporary
>>>> > > array in
>>>> dll_build_name/dll_locate_lib, and no
>>>> need to call free()
>>>>
>>>> so
>>>>
>>>> no
>>>> > > cleanup-related control
>>>> flow changes in these functions.
>>>> > >
>>>> > >
>>>> > >
>>>> > > =====
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>>
>>>>
>>> dllBuildName/webrev.02/src/os/windows/vm/os_windows.cpp.udiff.html
>>>>
>>>> > >
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>>
>>>>
>>>>
>>> dllBuildName/webrev.02/src/os/windows/vm/os_windows.cpp.udiff.html>
>>>>
>>>> > >
>>>> > >
>>>> > >
>>>> > > + int fullfnamelen =
>>>> strlen(JNI_LIB_PREFIX) +
>>>> strlen(fname) +
>>>> > > strlen(JNI_LIB_SUFFIX);
>>>> > >
>>>> > >
>>>> > >
>>>> > > int -> size_t (does
>>>> that even compile without warning?)
>>>> > >
>>>> > >
>>>> > >
>>>> > > + // Check current
>>>> working directory.
>>>> > >
>>>> > > + const char* p =
>>>> get_current_directory(buffer, buflen);
>>>> > >
>>>> > > + if (p != NULL &&
>>>> > >
>>>> > > +
>>>> strlen(buffer)
>>>> + 1 + fullfnamelen + 1 <= buflen) {
>>>> > >
>>>> > > + strcat(buffer,
>>>> "\\");
>>>> > >
>>>> > > + strcat(buffer,
>>>> fullfname);
>>>> > >
>>>> > > + retval =
>>>> file_exists(buffer);
>>>> > >
>>>> > >
>>>> > >
>>>> > > Small nit: I'd use
>>>> jio_snprintf instead of strcat.
>>>> Functionally
>>>> identical but
>>>> > > will make scanners (e.g.
>>>> coverity) happy. One could then avoid
>>>>
>>>> the
>>>>
>>>> length
>>>> > > calculation and rely on
>>>> jio_snprintf truncation:
>>>> > >
>>>> > >
>>>> > >
>>>> > > const char* p =
>>>> get_current_directory(buffer, buflen);
>>>> > >
>>>> > > if (p != NULL) {
>>>> > >
>>>> > > const size_t end =
>>>> strlen(p);
>>>> > >
>>>> > > if
>>>> (jio_snprintf(end,
>>>> buflen - end, "\\%s", fullname) !=
>>>> -1) {
>>>> > >
>>>> > > retval =
>>>> file_exists(buffer);
>>>> > >
>>>> > > }
>>>> > >
>>>> > > }
>>>> > >
>>>> > >
>>>> > >
>>>> > > --
>>>> > >
>>>> > >
>>>> > >
>>>> > > Not your change, but:
>>>> why does the code in
>>>> os::dll_locate_lib()
>>>> even
>>>> > > differentiate between a
>>>> PATH containing no
>>>> os::path_separator()
>>>> and a path
>>>> > > containing
>>>> os::path_separator()?
>>>> > >
>>>> > >
>>>> > >
>>>> > > Would the former
>>>> not be
>>>> just a PATH with only one directory
>>>>
>>>> and
>>>>
>>>> hence
>>>> > > need no special
>>>> treatment?
>>>> > >
>>>> > >
>>>> > >
>>>> > > =====
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>>
>>> dllBuildName/webrev.02/src/os/posix/vm/os_posix.cpp.udiff.ht
>>>> <http://os_posix.cpp.udiff.ht>ml
>>>>
>>>> > >
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>>
>>> dllBuildName/webrev.02/src/os/posix/vm/os_posix.cpp.udiff.ht
>>>> <http://os_posix.cpp.udiff.ht>ml>
>>>>
>>>> > >
>>>> > >
>>>> > >
>>>> > > Could
>>>> os::dll_locate_lib be consolidated
>>>> between windows and
>>>> unix?
>>>> > > Seems to be the
>>>> implementation is almost identical.
>>>> > >
>>>> > >
>>>> > >
>>>> > > ====
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>>
>>> dllBuildName/webrev.02/src/share/vm/prims/jvmtiExport.cpp.udiff.html
>>>>
>>>> > >
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>>
>>>>
>>> dllBuildName/webrev.02/src/share/vm/prims/jvmtiExport.cpp.udiff.html>
>>>>
>>>> > >
>>>> > >
>>>> > >
>>>> > > + // not
>>>> found -
>>>> try library path
>>>> > >
>>>> > >
>>>> > >
>>>> > > Proposal: "not
>>>> found -
>>>> try OS default library path"
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > > Find some
>>>> comments inline:
>>>> > >
>>>> > >
>>>> > > >
>>>> Especially if the path is empty, it
>>>> just returns 'true'.
>>>> > > >
>>>> Dll_build_name is usually used
>>>> before
>>>> calling dll_load.
>>>> If
>>>> > > dll_load does not get a
>>>> full path it searches
>>>> > > > in
>>>> well
>>>> known unix/windows locations. This is
>>>> intended
>>>> in
>>>> > > the two cases where
>>>> dll_build_name
>>>> > > > is
>>>> called with an empty path.
>>>> > > >
>>>> > > > So, for
>>>> both
>>>> cases (thread.cpp, jvmtiExport.cpp),
>>>> > > >
>>>> > > > before, we
>>>> would call os::dll_build_name()
>>>> with an
>>>> empty
>>>> > > string for the path
>>>> > > > which, for
>>>> relative paths, would result in
>>>> feeding
>>>> that path
>>>> > > unexpanded to
>>>> > > > dlopen(),
>>>> which would use whatever the OS
>>>> does in
>>>> those
>>>> > > cases (LIBPATH,
>>>> > > >
>>>> LD_LIBRARY_PATH, PATH on windows).
>>>> Note
>>>> that this
>>>>
>>>> does
>>>>
>>>> > > not necessarily
>>>> > > > include
>>>> searching the current directory.
>>>> > > Right. With
>>>> changed dll_biuld_name it's again
>>>> exactly as
>>>> > > before.
>>>> > >
>>>> > > > With your
>>>> change, we now use java.library.path,
>>>> which is
>>>> not
>>>> > > necessarily the
>>>> > > > same?
>>>> > > You are
>>>> right,
>>>> I oversaw that java.library.path
>>>> can be
>>>> > > overwritten. Initially,
>>>> > > it's set
>>>> to the
>>>> right thing.
>>>> > >
>>>> > > > (BTW, I
>>>> think
>>>> the old comments in thread.cpp and
>>>> > > jniExport.cpp were
>>>> wrong:"//
>>>> > > > Try the
>>>> local
>>>> directory" - if "local" means
>>>> "current",
>>>> this is
>>>> not
>>>> > > what did
>>>> > > > happen).
>>>> > > Right, I
>>>> tried
>>>> to adapt them, did I miss one?
>>>> > >
>>>> > > > I
>>>> added
>>>> a second variant of dll_build_name
>>>> without
>>>>
>>>> the
>>>>
>>>> > > path argument that
>>>> adds the
>>>> path
>>>> > > > from
>>>> system property java.lang.path and use
>>>> that in
>>>> these
>>>> > > two cases.
>>>> > > > I
>>>> changed the original function to
>>>> actually check file
>>>> > > availability in all
>>>> cases,
>>>> > > >
>>>> and to
>>>> check . if the path is empty.
>>>> > > > I think
>>>> that
>>>> may be a bit confusing. We would
>>>> then have
>>>> three
>>>> > > options:
>>>> > > >
>>>> > > > - call
>>>> os::dll_build_name with a real
>>>> "<aa>;<bb>;.." PATH
>>>> and
>>>> > > get a file name
>>>> > > > resolved
>>>> from
>>>> that path
>>>> > > > - call
>>>> os::dll_build_name with "" for the
>>>> PATH
>>>> and get OS
>>>> dll
>>>> > > resolution
>>>> > > No, in that
>>>> case, as I called file_exists(), it
>>>> would only work if
>>>> > > the dll is in the
>>>> > > current
>>>> working
>>>> directory. But I changed this now,
>>>> anyways.
>>>> > >
>>>> > > > - call your
>>>> new overloaded version of
>>>>
>>>> os::dll_build_name(),
>>>>
>>>> > > which uses -
>>>> > > >
>>>> Djava.library.path.
>>>> > > >
>>>> > > >
>>>> Please
>>>> review this change. I please need a
>>>> sponsor.
>>>> > > >
>>>>
>>>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> >
>>>> > > >
>>>> dllBuildName/webrev.01/
>>>> > >
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> > >
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>>
>>>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->>
>>>> >
>>>> > > >
>>>> dllBuildName/webrev.01/>
>>>> > >
>>>> > > >
>>>> > > > Best
>>>> regards,
>>>> > > >
>>>> Goetz.
>>>> > > >
>>>> > > >
>>>> > > >
>>>> > > >
>>>> > > > Kind
>>>> Regards,
>>>> Thomas
>>>> > >
>>>> > >
>>>> > >
>>>> > > Best Regards, Thomas
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>> > >
>>>>
>>>>
>>>>
>>>>
>>>>