[PATCH]: finish .cpu support for ARM

[Roman Divacky]

On Wed, Dec 03, 2014 at 07:18:31PM +0000, Renato Golin wrote:
> On 3 December 2014 at 18:52, Roman Divacky <rdivacky at vlakno.cz> wrote:
> > Can you explain why you think testing all variants is useful? Note that
> > the switching depends on already existing (and I think already tested)
> > components.
> 
> That's the problem, it doesn't. There are not many other tests, all of

What do you mean it doesnt? The patch uses ComputeAvailableFeatures()
and setAvailableFeatures() which are used in many other places.

Also, I really dont see how testing _all_ cpus will help testing
those two functions.

> them too similar to yours to make a difference. Basically, you're
> adding two things:
> 
> 1. Validating the CPU name, which is a small change and you have a
> test for that.
> 2. Adding the feature bits, which has nothing to do with your tests
> and are being included.
> 
> This is a new behaviour, and one that changes the semantics of
> assembly files quite considerably. Have you read the bugs I sent you?
> You should also have included the FIXME I asked. This is not a trivial
> matter, even though it's a trivial patch.
> 
> The new behaviour is that functions that do not have .cpu directives
> in it will start having it. Consider the case where I'm compiling to
> ARMv4, but have inline asm on one function that has ".cpu cortex-a15"
> to enable hardware divide, because "you know" that only in that case
> that function will be executed.
> 
> function HardDIV:
>   .cpu cortex-a15
>   hdiv whatever
>   bx lr
> function MiscompiledSoftDIV:
>   hdiv somethingelse
>   bx lr
> 
> Before your patch, only compiling the miscompiled function, the
> assembler would have found the error. Compiling both functions, you'd
> get the error you're trying to fix.
> 
> After your patch, only compiling the miscompiled function, the
> assembler would have found the error. Compiling both functions, you'll
> get no compiler error at all.
> 
> This means that, by adding a new cpu-specific function to your
> library, you lose the ability to detect miscompilations in the
> *entire* library.
> 
> Your patch is not testing for any of that, but it should. And it
> should include FIXMEs, too.
 
Yes, the patch changes the semantic of assembling files. How does testing
switching to all variants of cpus change anything about that? Or is this just
asking for a FIXME?

I have nothing against adding a FIXME (can you provide the text you want?) but
I dont see the need for testing all cpu variants.
 
Roman