Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 13/04/2018 16:09, Phil Blundell wrote:
> On Fri, 2018-04-13 at 13:42 -0300, Adhemerval Zanella wrote:
>> My understanding is glibc try support kernel with missing
>> functionalities but either using fallback implementations (either
>> subpar with underlying issues as for old pipe2), emulation (as for
>> copy_file_range which tries to emulate the kernel behaviour), or by
>> just warning userland the kernel do not provide the underlying
>> support (for instance set_robust_list).
> 
> It's true that glibc has tried (with varying levels of success) to
> provide compatibility implementations so that new APIs can be used on
> older kernels which lack some or other functionality.  But this doesn't
> extend to working around arbitrarily broken kernel configurations.
> 
> The cost of enabling CONFIG_ARM_THUMB in your kernel if you're building
> for ARMv7 anyway is so close to zero as to be completely negligible. 
> There is simply no rational reason to not have it included.  It would
> be interesting to ask the guy who filed that bugzilla ticket whether he
> has turned it off deliberately, and if so why.  I suspect he probably
> just didn't realise it was needed, rather than actively wanting it off.

Fair enough, looks like CONFIG_ARM_THUMB is not defined only for some
old armv5t kernel config. 

> 
>> I really think just saying 'go fix your kernel' is not the correct
>> answer, even more when the configuration used is supported upstream
>> (it not an experimental or out-of-tree one). 
> 
> There are any number of other ways in which you can break your system
> by configuring your kernel wrongly.  The fact that the kernel config
> system lets you turn a given option on or off doesn't mean you can just
> flip switches at random and expect things to work.  That said, in the
> particular case of CONFIG_ARM_THUMB, I think the kernel people should
> simply force this on when CONFIG_CPU_V7 is selected.

I strong agree CONFIG_CPU_V7 should imply CONFIG_ARM_THUMB.

> 
>> Also for specific case, my wild guess is using ARM code path should
>> not shown in much difference and will prevent such possible issues.
> 
> In this particular case you're right, the ARM implementation is
> probably not going to perform very much worse, and now that you've
> fixed at least the obvious bugs I think it will probably work fine.  
> 
> My concern is about the precedent it sets for the future.  Right now,
> it clearly is not true that building glibc for ARMv7 with -marm will
> use only A32 instructions.  Further, as far as I can tell it has never
> been true, so there cannot be any existing users who are depending on
> this behaviour.  If we fix this bug in the way that you are proposing,
> we would be making an implicit promise that any future ARMv7-optimised
> assembly code is also sensitive to being compiled under -marm and will
> avoid Thumb2 instructions in that situation. 
> 
> So, all in all it seems we have a choice between:
> 
> - we just tell this one guy "sorry, you have to enable CONFIG_ARM_THUMB
> in your kernel for glibc to work on ARMv7", he enables the option, it
> doesn't cost him anything, and everyone moves on; or
> 
> - we fix glibc to avoid Thumb2 instructions in these assembly files,
> which means we now have an extra variant that we have to maintain and
> test, we need to remember to add the same checks to any future assembly
> code that might be added for v7 in the future, and we essentially can't
> ever stop doing that for fear that users might have become dependent on
> it
> 
> I would prefer to do the former.
> 
> p.
> 

I do prefer the former and this discussion indeed shown there is little
gain in maintaining two build modes for the string functions.  I will
resend the patch with just removing the ARM code path and make thumb
as default an only build option.