[llvm-commits] [llvm] r165654 - in /llvm/trunk/lib: Support/CMakeLists.txt Support/Makefile VMCore/CMakeLists.txt VMCore/Makefile

[Sean Silva]

Hi all, sorry for the fallout. In the interest of unbreaking the bots as
fast as possible, I pushed out this revert quickly without a proper
analysis of the bug. Here is the analysis:

Post-Mortem Bug Analysis
========================

Cause of the bug
----------------

The bug was caused by certain object files being built with RTTI even
though in the build files the REQUIRES_RTTI flag had been deleted.

d0k (Benjamin Kramer) tracked down the following snippet in Makefile.rules:

  # IF REQUIRES_EH=1 is specified then don't disable exceptions
  ifndef REQUIRES_EH
    CXX.Flags += -fno-exceptions
  else
    # If the library requires EH, it also requires RTTI.
    REQUIRES_RTTI := 1
  endif

In other words, even though the REQUIRES_RTTI flag was not being set in the
Makefiles affected by the faulty commit, the REQUIRES_EH inside TableGen's
Makefile was nonetheless still causing REQUIRES_RTTI to be set.

The faulty commit should be able to be re-commited once TableGen no longer
relies on exceptions and the REQUIRES_EH flag is removed.

What initially indicated the presence of the bug
------------------------------------------------

Builbot failures across the board started appearing quickly after
committing the patch series which introduced the faulty commit.

How the bug slipped in
----------------------

The failures manifested themselves neither during my pre-commit testing nor
during my testing throughout development. This is because my local build is
with CMake/Ninja, and the above snippet from Makefile.rules does not appear
to have an analogue in the CMake build (this inconsistency may be
indicative of build bug and should be investigated by someone having more
experience with the build).

Another contributor to the presence of the bug may have been the following
code snippet from lib/Support/Makefile:

  ## FIXME: This only requires RTTI because tblgen uses it.  Fix that.
  REQUIRES_RTTI = 1

The presence of the comment made the change a lot more "obvious" than it
should have been, possibly inducing a slightly less reviewed, thought-out,
and tested commit than would otherwise have been the case. The above
snippet was introduced in r43127, approximately 3 years before r94376 which
introduced the code that unexpectedly set REQUIRES_RTTI in Makefile.rules.
I cannot think of a reasonable way to prevent the unfortunate interaction
of these two revisions, given the chronological extent over which they have
interacted.

How to prevent the bug in the future
------------------------------------

This bug would have been prevented if I had locally performed a Makefile
build before committing. This suggests that the following rule of thumb was
violated:

  If a commit touches a file, be sure to run a build which relies on that
  file.

In the case of the bug at hand, this rule of thumb was violated in that a
Makefile was touched, but the testing for the commit did not involve a
build that relied on the Makefile. This is obvious in retrospect: I would
not expect to commit changes to a file of Windows-only code without having
at least run a build on Windows, for example.

-- Sean Silva

On Wed, Oct 10, 2012 at 4:50 PM, Sean Silva <silvas at purdue.edu> wrote:
> Author: silvas
> Date: Wed Oct 10 15:50:36 2012
> New Revision: 165654
>
> URL: http://llvm.org/viewvc/llvm-project?rev=165654&view=rev
> Log:
> Revert r165652: "Remove unnecessary RTTI from the build."
>
> ... Apparently the RTTI is still necessary for some reason.
>
> Modified:
>     llvm/trunk/lib/Support/CMakeLists.txt
>     llvm/trunk/lib/Support/Makefile
>     llvm/trunk/lib/VMCore/CMakeLists.txt
>     llvm/trunk/lib/VMCore/Makefile
>
> Modified: llvm/trunk/lib/Support/CMakeLists.txt
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/CMakeLists.txt?rev=165654&r1=165653&r2=165654&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Support/CMakeLists.txt (original)
> +++ llvm/trunk/lib/Support/CMakeLists.txt Wed Oct 10 15:50:36 2012
> @@ -1,3 +1,5 @@
> +## FIXME: This only requires RTTI because tblgen uses it.  Fix that.
> +set(LLVM_REQUIRES_RTTI 1)
>  if( MINGW )
>    set(LLVM_REQUIRES_EH 1)
>  endif()
>
> Modified: llvm/trunk/lib/Support/Makefile
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Makefile?rev=165654&r1=165653&r2=165654&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Support/Makefile (original)
> +++ llvm/trunk/lib/Support/Makefile Wed Oct 10 15:50:36 2012
> @@ -11,6 +11,9 @@
>  LIBRARYNAME = LLVMSupport
>  BUILD_ARCHIVE = 1
>
> +## FIXME: This only requires RTTI because tblgen uses it.  Fix that.
> +REQUIRES_RTTI = 1
> +
>  EXTRA_DIST = Unix Win32 README.txt
>
>  include $(LEVEL)/Makefile.common
>
> Modified: llvm/trunk/lib/VMCore/CMakeLists.txt
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/VMCore/CMakeLists.txt?rev=165654&r1=165653&r2=165654&view=diff
> ==============================================================================
> --- llvm/trunk/lib/VMCore/CMakeLists.txt (original)
> +++ llvm/trunk/lib/VMCore/CMakeLists.txt Wed Oct 10 15:50:36 2012
> @@ -1,3 +1,5 @@
> +set(LLVM_REQUIRES_RTTI 1)
> +
>  add_llvm_library(LLVMCore
>    AsmWriter.cpp
>    Attributes.cpp
>
> Modified: llvm/trunk/lib/VMCore/Makefile
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/VMCore/Makefile?rev=165654&r1=165653&r2=165654&view=diff
> ==============================================================================
> --- llvm/trunk/lib/VMCore/Makefile (original)
> +++ llvm/trunk/lib/VMCore/Makefile Wed Oct 10 15:50:36 2012
> @@ -9,6 +9,7 @@
>  LEVEL = ../..
>  LIBRARYNAME = LLVMCore
>  BUILD_ARCHIVE = 1
> +REQUIRES_RTTI = 1
>
>  BUILT_SOURCES = $(PROJ_OBJ_ROOT)/include/llvm/Intrinsics.gen
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits