LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 20757 - Restrict ARM assembly header flags to the header and fix its behaviour for architecture attributes
Summary: Restrict ARM assembly header flags to the header and fix its behaviour for ar...
Status: RESOLVED WONTFIX
Alias: None
Product: libraries
Classification: Unclassified
Component: Backend: ARM (show other bugs)
Version: trunk
Hardware: PC Linux
: P normal
Assignee: Renato Golin
URL:
Keywords:
: 21319 (view as bug list)
Depends on: 20787
Blocks:
  Show dependency tree
 
Reported: 2014-08-26 15:27 PDT by Renato Golin
Modified: 2015-07-30 05:27 PDT (History)
8 users (show)

See Also:
Fixed By Commit(s):


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Renato Golin 2014-08-26 15:27:25 PDT
As extensively discussed in this thread:

https://sourceware.org/ml/binutils/2014-08/msg00119.html

Some architecture flags (fpu/cpu/arch/arch_extension/eabi_attribute) do not belong in the middle of assembly files. This includes hand-coded, inline and included assembly blocks which should not change the behaviour of the compile unit mid-way through the compilation process.

GNU folks are (understandably) reluctant to restrict this unintentional, and possibly destructive, functionality, even though they agree that this is not how it should be.

The technical consensus seems to be that those flags should be either restricted to the header (ie. before any code/text), or on a first come, first served basis, though the latter creates problems if the original file didn't have any flags and some include/inline asm added the wrong one. Current behaviour is last come, last served, which is plain wrong, if in text, but ok if in the header.

We can start by adding a warning that could be turned into an error (via -Werror), and see how it goes from there. If at least we warn, the user can't complain that further steps (linking, execution) fail without warning.

My proposal is the following:

1. Header directives shall warn when used in text and *only* change the behaviour for instruction selection purposes, not for module-wide properties (like build attributes).

2. If header directives are not present in the header, module-wide properties will be inferred from the command line, architectural descriptions, etc. even if they are present in the text.

3. The semantics of directives in text will be that they will change only the specific behaviour they're related to and they will be valid until another similar directive is issues or the end of file (compile unit) is reached.

4. Related flags shall compliment each other. So, .cpu cortex-a8 shall set VFP3 and NEON, while .fpu vfp2 shall only unset VFP3, not NEON and not move the CPU back to where VFP2 was the default.

5. Duplicate header directives in the header generates a *different* warning, but are accepted on a last come, last served basis.

6. Both warnings should become errors when in -Werror mode.

cheers,
--renato
Comment 1 Renato Golin 2014-08-28 08:26:12 PDT
Ignore item #4, as this is not what GAS does and I agree we should not do. Flags like vfp2 shall cancel NEON and CPU flags shall not change FP flags.
Comment 2 Renato Golin 2014-10-21 03:33:04 PDT
*** Bug 21319 has been marked as a duplicate of this bug. ***
Comment 3 Renato Golin 2014-10-22 03:25:18 PDT
*** Bug 21319 has been marked as a duplicate of this bug. ***
Comment 4 Renato Golin 2014-10-22 03:27:30 PDT
Just to be clear, moving the flags to the header won't fix the lack of attribute setting behaviour.

This bug is about fixing that behaviour without introducing unexpected behaviour, like setting file-level attributes on .fpu/.cpu/.arch flags across the file.
Comment 5 Renato Golin 2014-12-04 14:42:51 PST
We've discussed this in the US dev meeting and the consensus is that using push/pop like MIPS for directives is the best way forward. We hope that other compilers follow suit, but we'll still support the old syntax for a long time, maybe with a warning that this is bad behaviour.

As of r223147, cpu and fpu directives set the attribute bits, changing the properties of the file, failing to warn on real miscompilation errors, like:

function HardDIV:
  .cpu cortex-a15
  hdiv whatever
  bx lr

function MiscompiledSoftDIV:
  hdiv somethingelse
  bx lr

A better approach would be:

function HardDIV:
  .push cpu cortex-a15
  hdiv whatever
  .pop cpu
  bx lr
function MiscompiledSoftDIV:
  hdiv somethingelse
  bx lr

Or something similar, so that MiscompiledSoftDiv can correctly error out. This also allows *any* inline assembly code to use whatever cpu/fpu/isa they want and make sure that the context will be restored afterwards.

Implementing the old syntax will be an alias to .push without a pop. An alternative syntax created from that will be:

function HardDIV:
  .cpu cortex-a15
  hdiv whatever
  .pop cpu
  bx lr

Which is weird, but acceptable. We could error out, warn or even ignore. Or maybe even make this the default syntax. We need to discuss this further when time comes.
Comment 6 Renato Golin 2015-05-08 16:41:48 PDT
After long discussions, I think even if we do implement push/pop, the acceptance outside of Clang/LLVM will be zero, and that will create a problem for developers, that will end up ifdef'ing to go around LLVM vs. other compilers.

I think a compromise is to produce a warning when those flags are used after the header in assembly files or in inline assembly.
Comment 7 Renato Golin 2015-07-23 13:05:21 PDT
http://reviews.llvm.org/D11216
Comment 8 Renato Golin 2015-07-30 05:27:07 PDT
So, this seems to have little importance to everyone else, so I'll just abandon this revision. If anyone feels like it, they can continue from where I stopped.