Re: [PATCH v3 4/6] ArmPkg/Library/CompilerIntrinsicsLib: Enable VS2017/ARM builds

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 13 December 2017 at 14:56, Pete Batard <pete@akeo.ie> wrote:
> Hi Ard. Thanks for reviewing this.
>
> On 2017.12.12 19:59, Ard Biesheuvel wrote:
>>>
>>> +  EXPORT _fltused
>>> +  EXPORT __brkdiv0
>>> +
>>
>>
>> Why are these being exported? Are these part of the CRT ABI?
>
>
> Good point. I exported them because I was encountering undefined symbols for
> those when I started working on adding ARM support. But I now suspect they
> may have come from an export statement from VS headers (which we no longer
> include), as re-test shows that the exports seem to be superfluous.
>
> I will test further to confirm that this is the case, and remove the
> unneeded exports.
>
>>> +__brkdiv0
>>> +    udf #249
>>> +
>>
>>
>> This will cause a crash when invoked, in a way that gives very little
>> to go on to figure out what happens.
>
>
> Yes. I mostly copied what ReactOS was doing here, since it should be similar
> to what Microsoft does internally.
>
>> Perhaps branch to a C function
>> here that calls ASSERT() and CpuDeadLoop ?
>
>
> I don't mind giving it a try, if we keep the separate .asm files for MSVC.
>
> On the other hand, if I am to try to reuse existing assembly, as you suggest
> below, then I don't think I want to introduce anything that will differ from
> how divisions by zero are currently handled there. As far as I can tell (for
> RVCT, which is the only assembly that might be suitable for reuse with MSFT)
> the current way of handling a zero divisor is to set result and remainder to
> 0 and return (as per uldiv.asm). For gcc, we also have the following comment
> in div.S:
>
>   "@ What to do about division by zero?  For now, just return."
>
> So, while I agree that calling ASSERT and CpuDeadLoop is probably the better
> approach, I'd rather have the introduction of this behaviour be the subject
> of a separate ARM harmonization patch, that applies to all the toolchains,
> rather than something that's specific to Visual Studio usage.
>

I agree. Reusing code is more important than having one flavour of
division that will spot div-by-0. But we really don't want the UDF,
because it will crash unconditionally without a hint of what happened.

>> There are many different division implementations already in the same
>> directory. Do we really need a new one?
>
>
> I'll look into that. My goal with this series was to make sure that the
> changes being introduced would not break existing toolchains, which is why I
> saw it preferable to keep the MSVC intrinsics separate.
>
> Now, gcc assembly is not something that can be reused easily with the MS
> toolchain, so the only possibility are the RVCT .asm files.
>
> There's a fair bit of work involved into trying to figure out if and how we
> can reuse that code, but I'll see what I can do. It'll probably be a few
> weeks before I get back to this list on that however.
>

I'm prepared to be pragmatic here. I would prefer to have all
platforms run the same low level division code, but I understand that
it is tightly coupled to the compiler, given that they are intrinsics.

So yes, please try to reuse the RVCT code if feasible without a ton of
rework, but it seems like we will need to live with some MSFT specific
hooks anyway (if I understand the prototypes correctly)