This is an archive of the discontinued LLVM Phabricator instance.

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

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
MaskRay edited the summary of this revision. (Show Details)Jan 17 2021, 5:02 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?

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.

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.

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:)

" (+ 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 :-)

Is this good? :)

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

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

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

This revision was landed with ongoing or failed builds.Jan 20 2021, 2:22 PM
This revision was automatically updated to reflect the committed changes.
Harbormaster completed remote builds in B85958: Diff 317985.

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/

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.