[ARM] Warnings for .cpu/.fpu/.arch directives in ASM

[rengolin]

Summary

Reuse of architecture assembly directives is not an error, but can be
misleading and bring future problems, like lack of proper checking on
bad code generation, if the latter directive is less restricting than
the former, and the latter is only valid for one function.

One example is for IFUNC implementations, having multiple architectures
per compilation unit:

.fpu softvfp
func_soft_float:
  ...

func_vfp3:
  .fpu vfpv3
  ...

@ end of IFUNC block
...

.unrelated_func:
  @ assuming back to softvfp
  @ but .fpu vfpv3 leaked until here
  ...
  @ then we accidentally generate a VFP unstruction
  VMOV ...
  @ this should be an error, but the .fpu directive above masks it

The VMOV above could be a compiler code generation error, or could be a
user error, either way, we can't warn about this error, due to the leaky
nature of .fpu directives.

Another case is in inline assembly:

global_vfp = false;
my_c_func() {
  _asm_(".fpu vfpv3");
  // code that depends on VFP3
}

my_other_func() {
  if (global_vfp) {
    _asm_("VMOV ...");
  else
    _asm_("mov, mov...");
}

Here, there's an implicit contract between the C global_vfp variable and
the assmebly .fpu directive. If that contract is broken, there's no way
the compiler can warn the user, since the ".fpu vfpv3" will allow the
assembler to emit the VMOV regardless of what global_vfp is. This was
probably not the intention of the programmer when (s)he used the .fpu
directive in my_c_func().

Both examples are based on a broken design, but both appear in user code
often enough to be a problem. Multi-architecture machines and OSs are
becoming more common to exacerbate that problem, so a warning is due.

Fixes PR20757.

Problems:

• This will turn unconditional warnings when using "clang ... file.s" and may break compilations where -Wall and -Werror are set.
  • I could add an -mllvm -disable-asm-warnings for now, until we can link Clang warning infrastructure with the assembler
• This warns on misuse of directives, not on instruction selection issues (as outlined by the test).
  • There's no way to warn on codegen problems without changing the flags back to what's in the header, and that's a big change in behaviour from GAS.
• This patch doesn't warn on use of the directives in inline asm.
  • That's for another patch.

[• rafael]

Won't this warn on something as simple as

clang -c -flto -arch-opt-1 test1.c
clang -c -flto -arch-opt-2 test2.c
clang test1.o test2.o -o foo -Wl,-plugin-opt=save-temps
clang -c foo.s

?

Maybe the correct thing is to warn if the state is different at the start and end of an inline assembly?

[rengolin]

In D11216#207421, @rafael wrote:

clang -c -flto -arch-opt-1 test1.c
clang -c -flto -arch-opt-2 test2.c
clang test1.o test2.o -o foo -Wl,-plugin-opt=save-temps
clang -c foo.s

No, because when we output assembly files, they have the correct set of directives in the correct place.

This only happens in two cases:

1. Users add them to inline assembly inside functions and forget that it leaks to the rest of the file

2. Users add them to assembly files, inside functions and forget that it leaks to the rest of the file

Maybe the correct thing is to warn if the state is different at the start and end of an inline assembly?

This wouldn't catch the case where, accidentally, the user changes back to the original, because the order of directives happen to coincide with what you're compiling.

Example:

foo:
  .fpu neon
bar:
  .fpu vfpv3

This would "pass":

$ clang -S -mfpu=vfpv3 foo.s

But this will warn:

$ clang -S -mfpu=neon foo.s

Then, say, there's a code refactoring, and foo/bar get swapped places. Then the first warns and the second doesn't.

[rengolin]

Ping?

[rengolin]

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.

lib/Target/ARM/AsmParser/ARMAsmParser.cpp

     CantUnwindLocs = Locs();
     PersonalityLocs = Locs();
     HandlerDataLocs = Locs();
     PersonalityIndexLocs = Locs();
     FPReg = ARM::SP;
   }
 };
 
+// This structure keeps tabs on which arch modifying directives
+// have been used, and if the header is over, and the code has
+// started (first call to emit a function label).
+// We use this to warn on duplicated use of those directives, or
+// when they're used inside functions, since they leak the attribute
+// to the rest of the file (not always intentional).
+class ArchDirectiveMap {
+  SMLoc Arch;
+  SMLoc ArchExt;
+  SMLoc CPU;
+  SMLoc FPU;
+  bool Started = false;
+  MCAsmParser &Parser;
+
+  void check(SMLoc &Prev, SMLoc &Loc, Twine &Name) {
+    if (Prev.isValid()) {
+      Parser.Warning(Loc, Name + " used more than once");
+      Parser.Warning(Prev, Name + " previously defined here");
+    }
+    Prev = Loc;
+    if (Started)
+      Parser.Warning(Loc, Name + " used outside header");
+  }
+public:
+  ArchDirectiveMap(MCAsmParser &Parser) : Parser(Parser) {}
+  void checkArch(SMLoc &Loc) {
+    Twine Name(".arch");
+    check(Arch, Loc, Name);
+  }
+  void checkArchExt(SMLoc &Loc) {
+    Twine Name(".arch_extension");
+    check(ArchExt, Loc, Name);
+  }
+  void checkCPU(SMLoc &Loc) {
+    Twine Name(".cpu");
+    check(CPU, Loc, Name);
+  }
+  void checkFPU(SMLoc &Loc) {
+    Twine Name(".fpu");
+    check(FPU, Loc, Name);
+  }
+  void start() {
+    Started = true;
+  }
+};
+
 class ARMAsmParser : public MCTargetAsmParser {
   MCSubtargetInfo &STI;
   const MCInstrInfo &MII;
   const MCRegisterInfo *MRI;
   UnwindContext UC;
+  ArchDirectiveMap ADM;
 
   ARMTargetStreamer &getTargetStreamer() {
     assert(getParser().getStreamer().getTargetStreamer() &&
            "do not have a target streamer");
     MCTargetStreamer &TS = *getParser().getStreamer().getTargetStreamer();
     return static_cast<ARMTargetStreamer &>(TS);
   }
 

     Match_RequiresThumb2,
 #define GET_OPERAND_DIAGNOSTIC_TYPES
 #include "ARMGenAsmMatcher.inc"
 
   };
 
   ARMAsmParser(MCSubtargetInfo &STI, MCAsmParser &Parser,
                const MCInstrInfo &MII, const MCTargetOptions &Options)
-      : STI(STI), MII(MII), UC(Parser) {
+      : STI(STI), MII(MII), UC(Parser), ADM(Parser) {
     MCAsmParserExtension::Initialize(Parser);
 
     // Cache the MCRegisterInfo.
     MRI = getContext().getRegisterInfo();
 
     // Initialize the set of available features.
     setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
 

   return false;
 }
 
 void ARMAsmParser::onLabelParsed(MCSymbol *Symbol) {
   if (NextSymbolIsThumb) {
     getParser().getStreamer().EmitThumbFunc(Symbol);
     NextSymbolIsThumb = false;
   }
+  ADM.start();
 }
 
 /// parseDirectiveThumbFunc
 ///  ::= .thumbfunc symbol_name
 bool ARMAsmParser::parseDirectiveThumbFunc(SMLoc L) {
   MCAsmParser &Parser = getParser();
   const auto Format = getContext().getObjectFileInfo()->getObjectFileType();
   bool IsMachO = Format == MCObjectFileInfo::IsMachO;

   RegisterReqs.erase(Parser.getTok().getIdentifier().lower());
   Parser.Lex(); // Eat the identifier.
   return false;
 }
 
 /// parseDirectiveArch
 ///  ::= .arch token
 bool ARMAsmParser::parseDirectiveArch(SMLoc L) {
+  ADM.checkArch(L);
+
   StringRef Arch = getParser().parseStringToEndOfStatement().trim();
 
   unsigned ID = ARMTargetParser::parseArch(Arch);
 
   if (ID == ARM::AK_INVALID) {
     Error(L, "Unknown arch name");
     return false;
   }

   else if (IsStringValue)
     getTargetStreamer().emitTextAttribute(Tag, StringValue);
   return false;
 }
 
 /// parseDirectiveCPU
 ///  ::= .cpu str
 bool ARMAsmParser::parseDirectiveCPU(SMLoc L) {
+  ADM.checkCPU(L);
+
   StringRef CPU = getParser().parseStringToEndOfStatement().trim();
   getTargetStreamer().emitTextAttribute(ARMBuildAttrs::CPU_name, CPU);
 
   // FIXME: This is using table-gen data, but should be moved to
   // ARMTargetParser once that is table-gen'd.
   if (!STI.isCPUStringValid(CPU)) {
     Error(L, "Unknown CPU name");
     return false;
   }
 
   STI.setDefaultFeatures(CPU);
   setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
 
   return false;
 }
 /// parseDirectiveFPU
 ///  ::= .fpu str
 bool ARMAsmParser::parseDirectiveFPU(SMLoc L) {
+  ADM.checkFPU(L);
+
   SMLoc FPUNameLoc = getTok().getLoc();
   StringRef FPU = getParser().parseStringToEndOfStatement().trim();
 
   unsigned ID = ARMTargetParser::parseFPU(FPU);
   std::vector<const char *> Features;
   if (!ARMTargetParser::getFPUFeatures(ID, Features)) {
     Error(FPUNameLoc, "Unknown FPU name");
     return false;

   { ARM::AEK_IWMMXT2, Feature_None, {} },
   { ARM::AEK_MAVERICK, Feature_None, {} },
   { ARM::AEK_XSCALE, Feature_None, {} },
 };
 
 /// parseDirectiveArchExtension
 ///   ::= .arch_extension [no]feature
 bool ARMAsmParser::parseDirectiveArchExtension(SMLoc L) {
+  ADM.checkArchExt(L);
+
   MCAsmParser &Parser = getParser();
 
   if (getLexer().isNot(AsmToken::Identifier)) {
     Error(getLexer().getLoc(), "unexpected token");
     Parser.eatToEndOfStatement();
     return false;
   }
 

test/MC/ARM/directive-warning.s

+@ RUN: llvm-mc -triple armv7-eabi %s 2>&1 | FileCheck %s
+
+
+  .text
+  @ Ok, in header
+  .cpu cortex-a8
+  .fpu vfpv3
+  .globl  main
+  .align  2
+  .type  main,%function
+main:
+  @ VFP3, ok
+  VMOV.F64 d0, d1
+  bl  lr
+.Lfunc_end0:
+  .size  main, .Lfunc_end0-main
+
+  .globl  neon
+  .align  2
+  .type  neon,%function
+neon:
+  .fpu neon
+@ CHECK: directive-warning.s:{{.*}} warning: .fpu used more than once
+@ CHECK-NEXT: .fpu neon
+@ CHECK: directive-warning.s:{{.*}} warning: .fpu previously defined here
+@ CHECK-NEXT: .fpu vfpv3
+@ CHECK: directive-warning.s:{{.*}} warning: .fpu used outside header
+@ CHECK-NEXT: .fpu neon
+  @ NEON, ok, but warn
+  VMLA.U32 d1, d2, d3
+  bl  lr
+.Lfunc_end1:
+  .size  neon, .Lfunc_end1-neon
+
+  .globl  vfp3
+  .align  2
+  .type  vfp3,%function
+vfp3:
+  @ NEON, not ok, but can't warn
+  VMOV d0, d1
+  bl  lr
+.Lfunc_end2:
+  .size  vfp3, .Lfunc_end2-vfp3
+
+  @ Post-header declaration, not ok, warn
+  .arch_extension idiv
+  .cpu cortex-a15
+  .arch armv4
+@ CHECK: directive-warning.s:{{.*}} warning: .arch_extension used outside header
+@ CHECK: .arch_extension idiv
+@ CHECK: directive-warning.s:{{.*}} warning: .cpu used more than once
+@ CHECK: .cpu cortex-a15
+@ CHECK: directive-warning.s:{{.*}} warning: .cpu previously defined here
+@ CHECK: .cpu cortex-a8
+@ CHECK: directive-warning.s:{{.*}} warning: .cpu used outside header
+@ CHECK: .cpu cortex-a15
+@ CHECK: directive-warning.s:{{.*}} warning: .arch used outside header
+@ CHECK: .arch armv4