Eric / Saleem, The simple patch attached works out for two of the three cases I'm trying to do:
1. Multiple uses of .cpu/.fpu/.arch directives in asm files and inline asm; 2. Use of those directives after code has been emitted in asm files; But it doesn't cover the third case: 3. Use of those directives at all in inline assembly. There is a warning flag -Winline-asm which is enabled by default, and supporessing it (via -Wno-inline-asm) does stop warning (1) above, but I haven't figured out yet how. Warning uses PrintMessage with a type DK_Warning, but nothing stopping it from being printed. The code is a bit confusing. My aim is to: * Let -Winline-asm enabled by default, and warn (1) and (3) in inline asm unless disabled. * Add a new warning -W???, disabled by default, enabled by -Weverything or -Wextra, that will turn on (1) and (2) in asm files. I'll have to do that on the ARMAsmParser level, since anything more generic won't know the difference between 1, 2 and 3. Any ideas on how to get the warnings visible at that level? cheers, --renato
commit 2c416e8b6fc0448765a37b53aabae2c16a20b83c Author: Renato Golin <[email protected]> Date: Thu May 28 21:52:34 2015 +0100 Warnings for .cpu/.fpu/.arch directives in ASM 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. diff --git a/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/lib/Target/ARM/AsmParser/ARMAsmParser.cpp index 30c7d62..9eb840a 100644 --- a/lib/Target/ARM/AsmParser/ARMAsmParser.cpp +++ b/lib/Target/ARM/AsmParser/ARMAsmParser.cpp @@ -126,11 +126,58 @@ public: } }; +// 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() && @@ -346,7 +393,7 @@ public: 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. @@ -8876,6 +8923,7 @@ void ARMAsmParser::onLabelParsed(MCSymbol *Symbol) { getParser().getStreamer().EmitThumbFunc(Symbol); NextSymbolIsThumb = false; } + ADM.start(); } /// parseDirectiveThumbFunc @@ -9037,6 +9085,8 @@ bool ARMAsmParser::parseDirectiveUnreq(SMLoc L) { /// parseDirectiveArch /// ::= .arch token bool ARMAsmParser::parseDirectiveArch(SMLoc L) { + ADM.checkArch(L); + StringRef Arch = getParser().parseStringToEndOfStatement().trim(); unsigned ID = ARMTargetParser::parseArch(Arch); @@ -9165,6 +9215,8 @@ bool ARMAsmParser::parseDirectiveEabiAttr(SMLoc L) { /// parseDirectiveCPU /// ::= .cpu str bool ARMAsmParser::parseDirectiveCPU(SMLoc L) { + ADM.checkCPU(L); + StringRef CPU = getParser().parseStringToEndOfStatement().trim(); getTargetStreamer().emitTextAttribute(ARMBuildAttrs::CPU_name, CPU); @@ -9239,6 +9291,8 @@ static const struct { /// parseDirectiveFPU /// ::= .fpu str bool ARMAsmParser::parseDirectiveFPU(SMLoc L) { + ADM.checkFPU(L); + SMLoc FPUNameLoc = getTok().getLoc(); StringRef FPU = getParser().parseStringToEndOfStatement().trim(); @@ -10014,6 +10068,8 @@ static const struct { /// parseDirectiveArchExtension /// ::= .arch_extension [no]feature bool ARMAsmParser::parseDirectiveArchExtension(SMLoc L) { + ADM.checkArchExt(L); + MCAsmParser &Parser = getParser(); if (getLexer().isNot(AsmToken::Identifier)) {
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
