Makefile.rules: Avoid redundant .d generation (make restart) and inline archive rule to the only test

[MaskRay]

Summary

Take an example when CXX_SOURCES is main.cpp.

main.d is an included file. make will rebuild main.d, re-executes itself [1] to read
in the new main.d file, then rebuild main.o, finally link main.o into a.out.
main.cpp is parsed twice in this process.

This patch merges .d generation into .o generation [2], writes explicit rules
for .c/.m and deletes suffix rules for %.m and %.o. Since a target can be
satisfied by either of .c/.cpp/.m/.mm, we use multiple pattern rules. The
rule with the prerequisite (with VPATH considered) satisfied is used [3].

Since suffix rules are disabled, the implicit rule for archive member targets is
no long available [4]. Rewrite, simplify the archive rule and inline it into the
only test test/API/functionalities/archives/Makefile.

[1]: https://www.gnu.org/software/make/manual/html_node/Remaking-Makefiles.html
[2]: http://make.mad-scientist.net/papers/advanced-auto-dependency-generation/
[3]: https://www.gnu.org/software/make/manual/html_node/Pattern-Match.html
[4]: https://www.gnu.org/software/make/manual/html_node/Archive-Update.html

ObjC/ObjCXX tests only run on macOS. I don't have testing environment. Hope
someone can do it for me.

[MaskRay]

Don't use double-colon. https://lists.gnu.org/archive/html/help-make/2021-01/msg00016.html

[labath]

Looks like a nice cleanup. The only part I am not sure of is the part about removing $(RM) $(ARCHIVE_OBJECTS). Is that necessary?
I'm not sure why is that line there, but if I had to guess, I would say it's to ensure that lldb (on macos) reads debug info from the archive file instead of the original .o files. If it's not required, it may be better to leave it in. Otherwise, someone from Apple should say whether that is ok (testing archives is only really interesting on fruity platforms).

BTW, have you measured any speedups from these improvements?

[MaskRay]

In D94890#2505988, @labath wrote:

Looks like a nice cleanup. The only part I am not sure of is the part about removing $(RM) $(ARCHIVE_OBJECTS). Is that necessary?
I'm not sure why is that line there, but if I had to guess, I would say it's to ensure that lldb (on macos) reads debug info from the archive file instead of the original .o files. If it's not required, it may be better to leave it in. Otherwise, someone from Apple should say whether that is ok (testing archives is only really interesting on fruity platforms).

I can add back it under the ifeq "$(OS)" "Darwin" guard if Apple folks think it is useful.

BTW, have you measured any speedups from these improvements?

I don't notice a difference in check-lldb-api's Testing Time: . I think the time is probably dominated by running lldb and Python..

If the user builds -DBUILD_SHARED_LIBS=on clang and thus has a significant startup overhead, I guess there may be some differences.

[JDevlieghere]

In D94890#2507241, @MaskRay wrote:

In D94890#2505988, @labath wrote:

Looks like a nice cleanup. The only part I am not sure of is the part about removing $(RM) $(ARCHIVE_OBJECTS). Is that necessary?
I'm not sure why is that line there, but if I had to guess, I would say it's to ensure that lldb (on macos) reads debug info from the archive file instead of the original .o files. If it's not required, it may be better to leave it in. Otherwise, someone from Apple should say whether that is ok (testing archives is only really interesting on fruity platforms).

I can add back it under the ifeq "$(OS)" "Darwin" guard if Apple folks think it is useful.

It looks like we have only one test on llvm.org (+ one additional test in the Swift fork) that's using this. My vote is to just remove this together with the ARCHIVE_C_SOURCES, ARCHIVE_CXX_SOURCES, ARCHIVE_OBJC_SOURCES and ARCHIVE_OBJCXX_SOURCES and inline it in that one test.

[MaskRay]

In D94890#2507856, @JDevlieghere wrote:

In D94890#2507241, @MaskRay wrote:

In D94890#2505988, @labath wrote:

Looks like a nice cleanup. The only part I am not sure of is the part about removing $(RM) $(ARCHIVE_OBJECTS). Is that necessary?
I'm not sure why is that line there, but if I had to guess, I would say it's to ensure that lldb (on macos) reads debug info from the archive file instead of the original .o files. If it's not required, it may be better to leave it in. Otherwise, someone from Apple should say whether that is ok (testing archives is only really interesting on fruity platforms).

I can add back it under the ifeq "$(OS)" "Darwin" guard if Apple folks think it is useful.

It looks like we have only one test on llvm.org (+ one additional test in the Swift fork) that's using this. My vote is to just remove this together with the ARCHIVE_C_SOURCES, ARCHIVE_CXX_SOURCES, ARCHIVE_OBJC_SOURCES and ARCHIVE_OBJCXX_SOURCES and inline it in that one test.

Agree.

" (+ one additional test in the Swift fork)" --- Sounds like this can be a separate patch which should Swift folks a heads-up. I don't know how to test Swift and probably someone else can do it:)

[JDevlieghere]

In D94890#2507875, @MaskRay wrote:

" (+ one additional test in the Swift fork)" --- Sounds like this can be a separate patch which should Swift folks a heads-up. I don't know how to test Swift and probably someone else can do it:)

Yeah no worries, I'll take care of that :-)

[MaskRay]

Is this good? :)

[JDevlieghere]

In D94890#2510446, @MaskRay wrote:

Is this good? :)

I think you forgot to update the patch?

[MaskRay]

Inline archive rule

[MaskRay]

In D94890#2510627, @JDevlieghere wrote:

In D94890#2510446, @MaskRay wrote:

Is this good? :)

I think you forgot to update the patch?

Ah, looks like you want to do inline (ARCHIVE_OBJECTS) into the test in this patch... Done (it is a bit tricky).

[MaskRay]

Use $(AR)

Remove more ARCHIVE_NAME

[JDevlieghere]

LGTM with the inline comment addressed.

lldb/test/API/functionalities/archives/Makefile

11

I would remove the object files just to be sure.

[MaskRay]

$(RM) a.o b.o

[JDevlieghere]

Thanks!

[teemperor]

This is breaking the functionalities/archives/TestBSDArchives.py test on macOS. It seems the MAKE_DSYM flag somehow looses its effect when the dsym version of the test is running (and then we fail generating a dsym without input files): http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/27711/testReport/junit/lldb-api/functionalities_archives/TestBSDArchives_py/

[teemperor]

Not sure why we even run the DSYM variant if the test disables building DSYM. I just made this a no-debug-info-test in 060b51e0524aed6b6cc452baa8eb6d663a580eee which gets it running again on the bots for now.

lldb/packages/Python/lldbsuite/test/make/Makefile.rules

 #
 # Also might be of interest:
 # FRAMEWORK_INCLUDES (Darwin only) :=
 # CFLAGS_EXTRAS :=
 # LD_EXTRAS :=
 # SPLIT_DEBUG_SYMBOLS := YES
 # CROSS_COMPILE :=
 # USE_PRIVATE_MODULE_CACHE := YES
-#
-# And test/functionalities/archives/Makefile:
-# MAKE_DSYM := NO
-# ARCHIVE_NAME := libfoo.a
-# ARCHIVE_C_SOURCES := a.c b.c
 
 # Uncomment line below for debugging shell commands
 # SHELL = /bin/sh -x
 
+# Suppress built-in suffix rules. We explicitly define rules for %.o.
+.SUFFIXES:
+
 SRCDIR := $(shell dirname $(firstword $(MAKEFILE_LIST)))
 BUILDDIR := $(shell pwd)
 MAKEFILE_RULES := $(lastword $(MAKEFILE_LIST))
 THIS_FILE_DIR := $(shell dirname $(MAKEFILE_RULES))
 LLDB_BASE_DIR := $(THIS_FILE_DIR)/../../../../../
 
 # The test harness invokes the test Makefiles with an explicit 'all'
 # target, but its handy to be able to recursively call this Makefile
ifneq "$(strip $(OBJCXX_SOURCES))" ""
   OBJECTS +=$(strip $(OBJCXX_SOURCES:.mm=.o))
   CXX = $(call cxx_compiler,$(CC))
   LD = $(call cxx_linker,$(CC))
   ifeq "$(findstring lobjc,$(LDFLAGS))" ""
     LDFLAGS +=-lobjc
   endif
 endif
 
-#----------------------------------------------------------------------
-# Check if we have any C source files for archive
-#----------------------------------------------------------------------
-ifneq "$(strip $(ARCHIVE_C_SOURCES))" ""
-  ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_C_SOURCES:.c=.o))
-endif
-
-#----------------------------------------------------------------------
-# Check if we have any C++ source files for archive
-#----------------------------------------------------------------------
-ifneq "$(strip $(ARCHIVE_CXX_SOURCES))" ""
-  ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_CXX_SOURCES:.cpp=.o))
-  CXX = $(call cxx_compiler,$(CC))
-  LD = $(call cxx_linker,$(CC))
-endif
-
-#----------------------------------------------------------------------
-# Check if we have any ObjC source files for archive
-#----------------------------------------------------------------------
-ifneq "$(strip $(ARCHIVE_OBJC_SOURCES))" ""
-  ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_OBJC_SOURCES:.m=.o))
-  LDFLAGS +=-lobjc
-endif
-
-#----------------------------------------------------------------------
-# Check if we have any ObjC++ source files for archive
-#----------------------------------------------------------------------
-ifneq "$(strip $(ARCHIVE_OBJCXX_SOURCES))" ""
-  ARCHIVE_OBJECTS +=$(strip $(ARCHIVE_OBJCXX_SOURCES:.mm=.o))
-  CXX = $(call cxx_compiler,$(CC))
-  LD = $(call cxx_linker,$(CC))
-  ifeq "$(findstring lobjc,$(LDFLAGS))" ""
-    LDFLAGS +=-lobjc
-  endif
-endif
-
 ifeq ($(findstring clang, $(CXX)), clang)
   CXXFLAGS += --driver-mode=g++
 endif
 
 ifneq "$(CXX)" ""
   ifeq ($(findstring clang, $(LD)), clang)
     LDFLAGS += --driver-mode=g++
   endif
 endif
 
 #----------------------------------------------------------------------
 # DYLIB_ONLY variable can be used to skip the building of a.out.
 # See the sections below regarding dSYM file as well as the building of
 # EXE from all the objects.
 #----------------------------------------------------------------------
 
 #----------------------------------------------------------------------
 # Compile the executable from all the objects.
 #----------------------------------------------------------------------
 ifneq "$(DYLIB_NAME)" ""
 ifeq "$(DYLIB_ONLY)" ""
-$(EXE) : $(OBJECTS) $(ARCHIVE_NAME) $(DYLIB_FILENAME)
-  $(LD) $(OBJECTS) $(ARCHIVE_NAME) -L. -l$(DYLIB_NAME) $(LDFLAGS) -o "$(EXE)"
+$(EXE) : $(OBJECTS) $(DYLIB_FILENAME)
+  $(LD) $(OBJECTS) -L. -l$(DYLIB_NAME) $(LDFLAGS) -o "$(EXE)"
 ifneq "$(CODESIGN)" ""
   $(CODESIGN) -s - "$(EXE)"
 endif
 else
 EXE = $(DYLIB_FILENAME)
 endif
 else
-$(EXE) : $(OBJECTS) $(ARCHIVE_NAME)
-  $(LD) $(OBJECTS) $(LDFLAGS) $(ARCHIVE_NAME) -o "$(EXE)"
+$(EXE) : $(OBJECTS)
+  $(LD) $(OBJECTS) $(LDFLAGS) -o "$(EXE)"
 ifneq "$(CODESIGN)" ""
   $(CODESIGN) -s - "$(EXE)"
 endif
 endif
 
 #----------------------------------------------------------------------
 # Make the dSYM file from the executable if $(MAKE_DSYM) != "NO"
 #----------------------------------------------------------------------
 $(DSYM) : $(EXE)
 ifeq "$(OS)" "Darwin"
 ifneq "$(MAKE_DSYM)" "NO"
   "$(DS)" $(DSFLAGS) -o "$(DSYM)" "$(EXE)"
 else
 endif
 else
 ifeq "$(SPLIT_DEBUG_SYMBOLS)" "YES"
   $(OBJCOPY) --only-keep-debug "$(EXE)" "$(DSYM)"
   $(OBJCOPY) --strip-debug --add-gnu-debuglink="$(DSYM)" "$(EXE)" "$(EXE)"
 endif
 endif
 
 #----------------------------------------------------------------------
-# Make the archive
-#----------------------------------------------------------------------
-ifneq "$(ARCHIVE_NAME)" ""
-ifeq "$(OS)" "Darwin"
-$(ARCHIVE_NAME) : $(ARCHIVE_OBJECTS)
-  $(AR) $(ARFLAGS) $(ARCHIVE_NAME) $(ARCHIVE_OBJECTS)
-  $(RM) $(ARCHIVE_OBJECTS)
-else
-$(ARCHIVE_NAME) : $(foreach ar_obj,$(ARCHIVE_OBJECTS),$(ARCHIVE_NAME)($(ar_obj)))
-endif
-endif
-
-#----------------------------------------------------------------------
 # Make the dylib
 #----------------------------------------------------------------------
 $(DYLIB_OBJECTS) : CFLAGS += -DCOMPILING_LLDB_TEST_DLL
 
 ifneq "$(OS)" "Windows_NT"
 $(DYLIB_OBJECTS) : CFLAGS += -fPIC
 $(DYLIB_OBJECTS) : CXXFLAGS += -fPIC
 endif
ifeq "$(SPLIT_DEBUG_SYMBOLS)" "YES"
   $(OBJCOPY) --strip-debug --add-gnu-debuglink="$(DYLIB_FILENAME).debug" "$(DYLIB_FILENAME)" "$(DYLIB_FILENAME)"
 endif
 endif
 
 #----------------------------------------------------------------------
 # Make the precompiled header and compile C++ sources against it
 #----------------------------------------------------------------------
 
-#ifneq "$(PCH_OUTPUT)" ""
+ifneq "$(PCH_OUTPUT)" ""
 $(PCH_OUTPUT) : $(PCH_CXX_SOURCE)
   $(CXX) $(CXXFLAGS) -x c++-header -o $@ $<
-%.o : %.cpp $(PCH_OUTPUT)
-  $(CXX) $(PCHFLAGS) $(CXXFLAGS) -c -o $@ $<
-#endif
+endif
+
+%.o: %.c %.d
+  $(CC) $(CFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o: %.cpp %.d $(PCH_OUTPUT)
+  $(CXX) $(PCHFLAGS) $(CXXFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o: %.m %.d
+  $(CC) $(CFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
+
+%.o: %.mm %.d
+  $(CXX) $(CXXFLAGS) -MT $@ -MD -MP -MF $*.d -c -o $@ $<
 
 #----------------------------------------------------------------------
 # Automatic variables based on items already entered. Below we create
 # an object's lists from the list of sources by replacing all entries
 # that end with .c with .o, and we also create a list of prerequisite
 # files by replacing all .c files with .d.
 #----------------------------------------------------------------------
 PREREQS := $(OBJECTS:.o=.d)
-DWOS := $(OBJECTS:.o=.dwo) $(ARCHIVE_OBJECTS:.o=.dwo)
+DWOS := $(OBJECTS:.o=.dwo)
 ifneq "$(DYLIB_NAME)" ""
   DYLIB_PREREQS := $(DYLIB_OBJECTS:.o=.d)
   DYLIB_DWOS := $(DYLIB_OBJECTS:.o=.dwo)
 endif
 
-#----------------------------------------------------------------------
-# Rule for Generating Prerequisites Automatically using .d files and
-# the compiler -MM option. The -M option will list all system headers,
-# and the -MM option will list all non-system dependencies.
-#----------------------------------------------------------------------
-%.d: %.c
-  $(CC) -M $(CFLAGS) $< -MF $@ -MT $@ -MT $*.o
-
-%.d: %.cpp
-  @$(CXX) -M $(CXXFLAGS) $< -MF $@ -MT $@ -MT $*.o
-
-%.d: %.m
-  @$(CC) -M $(CFLAGS) $< -MF $@ -MT $@ -MT $*.o
-
-%.d: %.mm
-  @$(CXX) -M $(CXXFLAGS) $< -MF $@ -MT $@ -MT $*.o
+# Don't error if a .d file is deleted.
+$(PREREQS) $(DYLIB_PREREQS): ;
 
 #----------------------------------------------------------------------
 # Include all of the makefiles for each source file so we don't have
 # to manually track all of the prerequisites for each source file.
 #----------------------------------------------------------------------
-sinclude $(PREREQS)
-ifneq "$(DYLIB_NAME)" ""
-  sinclude $(DYLIB_PREREQS)
-endif
-
-# Define a suffix rule for .mm -> .o
-.SUFFIXES: .mm .o
-.mm.o:
-  $(CXX) $(CXXFLAGS) -c $<
+include $(wildcard $(PREREQS) $(DYLIB_PREREQS))
 
 .PHONY: clean
 dsym: $(DSYM)
 all:  $(EXE) $(DSYM)
 clean::
 ifeq "$(findstring lldb-test-build.noindex, $(BUILDDIR))" ""
   $(error Trying to invoke the clean rule, but not using the default build tree layout)
 else

lldb/test/API/functionalities/archives/Makefile

-C_SOURCES := main.c
-
+C_SOURCES := main.c a.c b.c
+EXE :=  # Define a.out explicitly
 MAKE_DSYM := NO
-ARCHIVE_NAME := libfoo.a
-ARCHIVE_C_SOURCES := a.c b.c
+
+all: a.out
+
+a.out: main.o libfoo.a
+  $(LD) $(LDFLAGS) $^ -o $@
+
+libfoo.a: a.o b.o
+  $(AR) $(ARFLAGS) $@ $^
+  $(RM) $^
 
 include Makefile.rules