This is an archive of the discontinued LLVM Phabricator instance.

Differential D94890

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

Authored by MaskRay on Jan 17 2021, 5:01 PM.

Details

Reviewers
friss
JDevlieghere
labath
rupprecht
Commits
rG6afdf13ae4cc: Makefile.rules: Avoid redundant .d generation (make restart) and inline archive…
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.

Diff Detail

Event Timeline

MaskRay requested review of this revision.Jan 17 2021, 5:01 PM
MaskRay created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptJan 17 2021, 5:01 PM
Herald added a subscriber: lldb-commits. · View Herald Transcript
MaskRay edited the summary of this revision. (Show Details)Jan 17 2021, 5:02 PM
Harbormaster completed remote builds in B85543: Diff 317248.Jan 17 2021, 5:31 PM
MaskRay updated this revision to Diff 317382.Jan 18 2021, 9:42 AM
MaskRay edited the summary of this revision. (Show Details)

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

Harbormaster completed remote builds in B85612: Diff 317382.Jan 18 2021, 10:09 AM
dblaikie added a subscriber: dblaikie.Jan 18 2021, 10:58 AM
labath added a comment.Jan 18 2021, 11:41 PM

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 added a comment.Jan 19 2021, 9:49 AM
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 added a comment.Jan 19 2021, 12:56 PM
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 added a comment.Jan 19 2021, 1:09 PM
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 added a comment.Jan 19 2021, 1:38 PM
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 added a comment.Jan 20 2021, 11:06 AM

Is this good? :)

JDevlieghere added a comment.Jan 20 2021, 12:19 PM
In D94890#2510446, @MaskRay wrote:

Is this good? :)

I think you forgot to update the patch?

MaskRay updated this revision to Diff 317983.Jan 20 2021, 12:59 PM

Inline archive rule

MaskRay added a comment.Jan 20 2021, 1:00 PM
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 updated this revision to Diff 317985.Jan 20 2021, 1:04 PM
MaskRay retitled this revision from Makefile.rules: Avoid redundant .d generation and make restart to Makefile.rules: Avoid redundant .d generation (make restart) and inline archive rule to the only test.
MaskRay edited the summary of this revision. (Show Details)

Use $(AR)

Remove more ARCHIVE_NAME

JDevlieghere accepted this revision.Jan 20 2021, 1:15 PM

LGTM with the inline comment addressed.

lldb/test/API/functionalities/archives/Makefile
11 ↗(On Diff #317985)

I would remove the object files just to be sure.

This revision is now accepted and ready to land.Jan 20 2021, 1:15 PM
MaskRay updated this revision to Diff 318011.Jan 20 2021, 2:20 PM
MaskRay marked an inline comment as done.

$(RM) a.o b.o

JDevlieghere accepted this revision.Jan 20 2021, 2:20 PM

Thanks!

This revision was landed with ongoing or failed builds.Jan 20 2021, 2:22 PM
Closed by commit rG6afdf13ae4cc: Makefile.rules: Avoid redundant .d generation (make restart) and inline archive… (authored by MaskRay). · Explain Why
This revision was automatically updated to reflect the committed changes.
MaskRay added a commit: rG6afdf13ae4cc: Makefile.rules: Avoid redundant .d generation (make restart) and inline archive….
Harbormaster completed remote builds in B85956: Diff 317983.Jan 20 2021, 3:45 PM
Harbormaster completed remote builds in B85958: Diff 317985.
Harbormaster completed remote builds in B85976: Diff 318011.Jan 20 2021, 5:40 PM
teemperor added a subscriber: teemperor.Jan 21 2021, 4:04 AM

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 mentioned this in rG060b51e0524a: [lldb] Make TestBSDArchives a no-debug-info-test.Jan 21 2021, 4:09 AM
teemperor added a comment.Jan 21 2021, 4:11 AM

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.

Revision Contents

PathSize
lldb/
packages/
Python/
lldbsuite/
test/
make/
59 lines

Diff 317248

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

Show First 20 Lines • Show All 564 Lines • ▼ Show 20 Linesifeq "$(SPLIT_DEBUG_SYMBOLS)" "YES"
$(OBJCOPY) --strip-debug --add-gnu-debuglink="$(DSYM)" "$(EXE)" "$(EXE)" $(OBJCOPY) --strip-debug --add-gnu-debuglink="$(DSYM)" "$(EXE)" "$(EXE)"
endif endif
endif endif
#---------------------------------------------------------------------- #----------------------------------------------------------------------
# Make the archive # Make the archive
#---------------------------------------------------------------------- #----------------------------------------------------------------------
ifneq "$(ARCHIVE_NAME)" "" ifneq "$(ARCHIVE_NAME)" ""
ifeq "$(OS)" "Darwin"
$(ARCHIVE_NAME) : $(ARCHIVE_OBJECTS) $(ARCHIVE_NAME): $(ARCHIVE_OBJECTS)
$(AR) $(ARFLAGS) $(ARCHIVE_NAME) $(ARCHIVE_OBJECTS) $(AR) $(ARFLAGS) $@ $^
$(RM) $(ARCHIVE_OBJECTS)
else
$(ARCHIVE_NAME) : $(foreach ar_obj,$(ARCHIVE_OBJECTS),$(ARCHIVE_NAME)($(ar_obj)))
endif
endif endif
#---------------------------------------------------------------------- #----------------------------------------------------------------------
# Make the dylib # Make the dylib
#---------------------------------------------------------------------- #----------------------------------------------------------------------
$(DYLIB_OBJECTS) : CFLAGS += -DCOMPILING_LLDB_TEST_DLL $(DYLIB_OBJECTS) : CFLAGS += -DCOMPILING_LLDB_TEST_DLL
ifneq "$(OS)" "Windows_NT" ifneq "$(OS)" "Windows_NT"
Show All 35 Linesifeq "$(SPLIT_DEBUG_SYMBOLS)" "YES"
$(OBJCOPY) --strip-debug --add-gnu-debuglink="$(DYLIB_FILENAME).debug" "$(DYLIB_FILENAME)" "$(DYLIB_FILENAME)" $(OBJCOPY) --strip-debug --add-gnu-debuglink="$(DYLIB_FILENAME).debug" "$(DYLIB_FILENAME)" "$(DYLIB_FILENAME)"
endif endif
endif endif
#---------------------------------------------------------------------- #----------------------------------------------------------------------
# Make the precompiled header and compile C++ sources against it # Make the precompiled header and compile C++ sources against it
#---------------------------------------------------------------------- #----------------------------------------------------------------------
#ifneq "$(PCH_OUTPUT)" "" ifneq "$(PCH_OUTPUT)" ""
$(PCH_OUTPUT) : $(PCH_CXX_SOURCE) $(PCH_OUTPUT) : $(PCH_CXX_SOURCE)
$(CXX) $(CXXFLAGS) -x c++-header -o $@ $< $(CXX) $(CXXFLAGS) -x c++-header -o $@ $<
%.o : %.cpp $(PCH_OUTPUT) endif
$(CXX) $(PCHFLAGS) $(CXXFLAGS) -c -o $@ $<
#endif .SUFFIXES:
%.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 # Automatic variables based on items already entered. Below we create
# an object's lists from the list of sources by replacing all entries # 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 # that end with .c with .o, and we also create a list of prerequisite
# files by replacing all .c files with .d. # files by replacing all .c files with .d.
#---------------------------------------------------------------------- #----------------------------------------------------------------------
PREREQS := $(OBJECTS:.o=.d) PREREQS := $(OBJECTS:.o=.d) $(ARCHIVE_OBJECTS:.o=.d)
DWOS := $(OBJECTS:.o=.dwo) $(ARCHIVE_OBJECTS:.o=.dwo) DWOS := $(OBJECTS:.o=.dwo) $(ARCHIVE_OBJECTS:.o=.dwo)
ifneq "$(DYLIB_NAME)" "" ifneq "$(DYLIB_NAME)" ""
DYLIB_PREREQS := $(DYLIB_OBJECTS:.o=.d) DYLIB_PREREQS := $(DYLIB_OBJECTS:.o=.d)
DYLIB_DWOS := $(DYLIB_OBJECTS:.o=.dwo) DYLIB_DWOS := $(DYLIB_OBJECTS:.o=.dwo)
endif endif
#---------------------------------------------------------------------- # Don't error if a .d file is deleted.
# Rule for Generating Prerequisites Automatically using .d files and $(PREREQS) $(DYLIB_PREREQS): ;
# 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
#---------------------------------------------------------------------- #----------------------------------------------------------------------
# Include all of the makefiles for each source file so we don't have # 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. # to manually track all of the prerequisites for each source file.
#---------------------------------------------------------------------- #----------------------------------------------------------------------
sinclude $(PREREQS) include $(wildcard $(PREREQS) $(DYLIB_PREREQS))
ifneq "$(DYLIB_NAME)" ""
sinclude $(DYLIB_PREREQS)
endif
# Define a suffix rule for .mm -> .o
.SUFFIXES: .mm .o
.mm.o:
$(CXX) $(CXXFLAGS) -c $<
.PHONY: clean .PHONY: clean
dsym: $(DSYM) dsym: $(DSYM)
all: $(EXE) $(DSYM) all: $(EXE) $(DSYM)
clean:: clean::
ifeq "$(findstring lldb-test-build.noindex, $(BUILDDIR))" "" ifeq "$(findstring lldb-test-build.noindex, $(BUILDDIR))" ""
$(error Trying to invoke the clean rule, but not using the default build tree layout) $(error Trying to invoke the clean rule, but not using the default build tree layout)
else else
Show All 17 Lines