[PATCH] D19410: [scan-build] fix warnings emitted on LLVM ARM code base

Saleem Abdulrasool via llvm-commits llvm-commits at lists.llvm.org
Thu May 5 09:02:56 PDT 2016


compnerd added inline comments.

================
Comment at: lib/Target/ARM/ARMFrameLowering.cpp:1610
@@ -1609,4 +1609,3 @@
   //        worth the effort and added fragility?
-  bool BigStack = (RS && (MFI->estimateStackSize(MF) +
-                              ((hasFP(MF) && AFI->hasStackFrame()) ? 4 : 0) >=
-                          estimateRSStackSizeLimit(MF, this))) ||
+  bool BigStack = ((MFI->estimateStackSize(MF) +
+                    ((hasFP(MF) && AFI->hasStackFrame()) ? 4 : 0) >=
----------------
rengolin wrote:
> Better ask @weimingz about this one.
Ugh, this is *subtle*.  If we are going to simplify this, please add a couple of asserts.

    assert(requiresRegisterScavenging() && "ARM requires register scavenging");
    assert(RS && "Target requiring register scavenging not provided register scavenger");

================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:9965
@@ +9964,3 @@
+  const MCSection *Section = getStreamer().getCurrentSection().first;
+  assert(Section && "must have section to emit alignment");
+  if (Section->UseCodeAlign())
----------------
rengolin wrote:
> We don't want asserts in assembly parsers, people can write pretty much anything. Make this an error.
Yes, adding an error here would be reasonable, however, if we do that, please add a test case that demonstrates the invalid input.  I believe that the assertion here is valid as we will always have a section (even if the user inserts it in the wrong location, theres the implicit initial text section to which we will refer).


http://reviews.llvm.org/D19410





More information about the llvm-commits mailing list