[sword-devel] SVN 3813 makes compilation of Sword fail

[Jaak Ristioja]

Ah indeed, thanks for correcting my incorrect reasoning I did late last 
night! But you are still relying on implementation-defined behavior here 
which might not work for every platform, and there might not even be an 
explicit guarantee it will continue to work for your platform.

As far as I can tell, the safest way is to pass to the callback the 
pointer to int. I think it would be valid in 
FTPLibFTPTransport::getURL() to pass &fd, e.g.:

   FtpOptions(FTPLIB_CALLBACK_WRITERARG, (long)fd, ftpConnection);

and in my_filewriter() just convert the (void*) argument to (int*) and, 
and dereference the pointer:

   int output = *static_cast<int *>(fd);

I believe this would avoid the implementation-defined behavior. As far 
as I can tell, the my_filewriter() callback is only called when inside 
the getURL() function, hence fd is always reachable and valid when 
my_filewriter() is called by FTPLib.

Best regards,
J

On 19.10.20 09:32, Troy A. Griffitts wrote:
> Hi Jaak,
> 
> We're not storing a pointer here. FTPLib gives us a void * we can do whatever we want with which we give them on initialization of a file transfer and they simply pass this back to us on write calls and status updates. We're storing the file handle there and we know it's an int and thus we explicitly cast it to an int. I don't mind a warning here, even with the explicit cast (which I assumed would, and I think should, skip the warning) but the compiler actually throws a compile error here, which is just stupid. I had to first cast the pointer to a size_t and then to an int to avoid the compiler error.
> 
> 
> On October 19, 2020 1:07:48 AM GMT+02:00, Jaak Ristioja <jaak at ristioja.ee> wrote:
>> "Added extra cast (int)(size_t) to avoid stupid clang error that
>> doesn't
>> like void * being cast (int) directly to an int."
>>
>> UH-OH!!!
>>
>> First of all, the build that failed for BibleTime was using GCC not
>> Clang.
>>
>> Secondly, the compiler is correct to warn, because a pointer does not
>> always fit into an int, e.g. for the LLP64 an LP64 data models [1]. So
>> it seems that you might be throwing away half of the bits and expect it
>>
>> to always work. For those data models it might work if your OS only
>> gives you addresses in the range of [0, 2^32) but that is not always
>> guaranteed, leading to undefined behavior.
>>
>> In short, please don't store pointers in integers other than intptr_t
>> and uintptr_t and convert them using reinterpret_cast! See also:
>>
>>    http://eel.is/c++draft/expr.reinterpret.cast#5
>>    http://eel.is/c++draft/basic.stc.dynamic.safety
>>
>> Best regards,
>> J
>>
>> [1] https://en.wikipedia.org/wiki/64-bit_computing#64-bit_data_models
>>
>>
>>
>> On 19.10.20 01:23, Troy A. Griffitts wrote:
>>> Crapola.  Should be fixed.  This was intended to be a call to the new
>>
>>> FileMgr::write which hides the OS-specific impl, but I was configured
>> to
>>> use CURL instead of FTPLib, so I missed the compilation error.  The
>>> Android port uses FTPLib and I just built there successfully with the
>>
>>> committed I just pushed.  Thank you Jaak.
>>>
>>> Troy
>>>
>>>
>>> On 10/18/20 11:57 PM, Jaak Ristioja wrote:
>>>> Hello!
>>>>
>>>> The commit "A bit more work on making it easier to use SWORD in a
>>>> threadsafe manner." makes compilation of Sword fail:
>>>>
>>>> src/mgr/ftplibftpt.cpp: In function ‘int
>>>> sword::{anonymous}::my_filewriter(netbuf*, void*, size_t, void*)’:
>>>> src/mgr/ftplibftpt.cpp:52:21: error: cast from ‘void*’ to ‘int’
>> loses
>>>> precision [-fpermissive]
>>>>     int output = (int)fd;
>>>>                       ^~
>>>> src/mgr/ftplibftpt.cpp: In function ‘int
>>>> sword::{anonymous}::my_filewriter(netbuf*, void*, size_t, void*)’:
>>>> src/mgr/ftplibftpt.cpp:52:21: error: cast from ‘void*’ to ‘int’
>> loses
>>>> precision [-fpermissive]
>>>>     int output = (int)fd;
>>>>                       ^~
>>>> src/mgr/ftplibftpt.cpp:53:3: error: ‘write’ was not declared in this
>>
>>>> scope
>>>>     write(output, buffer, size);
>>>>     ^~~~~
>>>> src/mgr/ftplibftpt.cpp:53:3: error: ‘write’ was not declared in this
>>
>>>> scope
>>>>     write(output, buffer, size);
>>>>     ^~~~~
>>>> src/mgr/ftplibftpt.cpp:53:3: note: suggested alternative: ‘fwrite’
>>>>     write(output, buffer, size);
>>>>
>>>> See https://github.com/bibletime/bibletime/runs/1272250545 for the
>>>> failing build run.
>>>>
>>>> Commit details in our git mirror of the Sword SVN repository:
>>>>
>>   https://github.com/bibletime/crosswire-sword-mirror/commit/c52559ecae
>>>>
>>>>
>>>> Best regards,
>>>> J
>>>> _______________________________________________
>>>> sword-devel mailing list: sword-devel at crosswire.org
>>>> http://crosswire.org/mailman/listinfo/sword-devel
>>>> Instructions to unsubscribe/change your settings at above page
>>> _______________________________________________
>>> sword-devel mailing list: sword-devel at crosswire.org
>>> http://crosswire.org/mailman/listinfo/sword-devel
>>> Instructions to unsubscribe/change your settings at above page
>>
>> _______________________________________________
>> sword-devel mailing list: sword-devel at crosswire.org
>> http://crosswire.org/mailman/listinfo/sword-devel
>> Instructions to unsubscribe/change your settings at above page
> 
> 
> _______________________________________________
> sword-devel mailing list: sword-devel at crosswire.org
> http://crosswire.org/mailman/listinfo/sword-devel
> Instructions to unsubscribe/change your settings at above page
>