Re: [hatari-devel] Portability patch

[Andreas Grabher <andreas_g86@xxxxxxxxxx>]

There is a bug in the run-loops affecting the C implementation of try/catch:
Before returning after non-zero do_specialties() we need to call STOPTRY. That call is missing. C++ WinUAE is not affected.

> Am 08.03.2021 um 22:29 schrieb Andreas Grabher <andreas_g86@xxxxxxxxxx>:
> 
> No problem.
> 
>> Am 08.03.2021 um 21:52 schrieb Nicolas Pomarède <npomarede@xxxxxxxxxxxxx>:
>> 
>> Le 08/03/2021 à 18:47, Andreas Grabher a écrit :
>>>>> Am 07.03.2021 um 19:07 schrieb Nicolas Pomarède <npomarede@xxxxxxxxxxxx>:
>>>>> 
>>>>> Le 27/02/2021 à 18:34, Andreas Grabher a écrit :
>>>>> Hello all,
>>>>> a nice fellow tested Previous with different compilers on different platforms and came up with a patch that should improve compatibility. I did apply all non-CPU related changes. But i did not apply everything that affects the code that Previous shares with WinUAE/Hatari because i don’t want that the code base drifts away too far. That causes lot of pain when applying patches.
>>>>> I have appended the patch. Maybe you want to have a look and apply it or parts of it to Hatari.
>>>>> Best wishes,
>>>>> Andreas
>>>> 
>>>> Hi
>>>> 
>>>> regarding the patch for the dsp part, it's mainly replacing sprintf with snprintf, but this makes the code a little "logner" to read ; it this really necessary ? These sprintf are used when doing disasm, but in that case it's unlikely the buffer in sprintf gets more bytes than, as the size of the sprintf are known (some register names and so on).
>>>> 
>>>> So, maybe it's just enough to have some large enough buffer.
>>>> patch  does this :
>>>> 
>>>> -static char str_instr[50];
>>>> -static char str_instr2[120];
>>>> +static char str_instr[96];
>>>> +static char str_instr2[192];
>>>> 
>>>> does it means that 50/120 are too small in some cases ; are there any real cases and if so, why the new values 96/192 ?
>>>> 
>>>> 
>>>> As for src/softfloat/softfloat.c , I leave to Toni to see if he wants to apply changes to main Winuae, if not I'd rather not have a different code in Hatari than the one from WinUAE.
>>>> 
>>>> 
>>>> For which platforms/compilers were those changes necessary ? Hatari is compiled with clang and gcc and there were no specific warnings recently.
>>>> 
>>>> Nicolas
>>>> 
>>>> Nicolas
>>>> 
>>>> 
>>> I asked the same question and he replied:
>>> „No, certainly nothing critical in there, they're mostly just to
>>> suppress compiler warnings, or because I consider sprintf() instead of
>>> snprintf() a needlessly risky practice, and to try to at least lay
>>> some groundwork for portability to architectures with unusual integer
>>> sizes.  I think they're all worth consideration in the upstream
>>> projects but it's probably not a high priority issue for anyone.“
>>> I think the most interesting part is in sysconfig.h. The changes in dsp code are mentioned above. The changes in softfloat.c are all about unused variables and mixing signed and unsigned integers in bitwise operations.
>> 
>> Hi
>> 
>> if there's no real case requiring these changes for now, then I don't think it's useful to add them to Hatari (we need to be able to replicate an issue to ensure a patch is correct and doesn't have counter effects).
>> 
>> regarding sysconfig.h, I think it's not really important if Previous or Hatari diverges from what WinUAE uses for example. It's really specific code, it can enable some optimisation or not and so on. So I think each emulator can taillor it to its need, not sure we need to have an unique one that fits all.
>> 
>> for sprintf/snprintf, this is a a personal taste to remove sprintf. With correct buffer size, I don't see why sprintf would be less safe. Also sprintf is used in many places in Hatari, not just dsp.c, so if we really want to remove them, it should be patched everywhere.
>> 
>> So, no offence to the original poster, but I'm not sure these patches are needed in Hatari at this point.
>> 
>> Nicolas
>> 
>> 
> 
>