Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New makefile to fix dependency errors #5546

Closed
wants to merge 1 commit into from
Closed

Conversation

Meiye-lj
Copy link

Hi,

Based on our study, we found some dependency errors. We have tried to fix them.

@chrchr-github
Copy link
Collaborator

Seems like dmake.cpp needs to be adapted.

@@ -515,7 +515,7 @@ $(libcppdir)/checkexceptionsafety.o: lib/checkexceptionsafety.cpp lib/addoninfo.
$(libcppdir)/checkfunctions.o: lib/checkfunctions.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checkfunctions.h lib/config.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkfunctions.cpp

$(libcppdir)/checkinternal.o: lib/checkinternal.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checkinternal.h lib/config.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h
$(libcppdir)/checkinternal.o: lib/checkinternal.cpp
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in my humble opinion this is wrong. the checkinternal.cpp includes various headers. The CHECK_INTERNAL is sometimes defined.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it was not compiled with CHECK_INTERNAL defined.

@@ -569,7 +569,7 @@ $(libcppdir)/color.o: lib/color.cpp lib/color.h lib/config.h
$(libcppdir)/cppcheck.o: lib/cppcheck.cpp externals/picojson/picojson.h externals/simplecpp/simplecpp.h externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/checkunusedfunctions.h lib/clangimport.h lib/color.h lib/config.h lib/cppcheck.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/json.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/preprocessor.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/timer.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/version.h lib/vfvalue.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/cppcheck.cpp

$(libcppdir)/ctu.o: lib/ctu.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/astutils.h lib/check.h lib/color.h lib/config.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h
$(libcppdir)/ctu.o: lib/ctu.cpp externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/astutils.h lib/check.h lib/color.h lib/config.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h externals/tinyxml2/tinyxml2.h
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The externals/tinyxml2/tinyxml2.h is already a dependency.

@firewave
Copy link
Collaborator

Based on our study, we found some dependency errors. We have tried to fix them.

Thanks for your contribution. Please provide more details on how you determined this and what issues this might fix.

FYI the Makefile is quite basic and lacks lots of features. You should use CMake if possible as it is way more complete (although it is still missing a few things from make).

@Meiye-lj
Copy link
Author

Based on our study, we found some dependency errors. We have tried to fix them.

Thanks for your contribution. Please provide more details on how you determined this and what issues this might fix.

FYI the Makefile is quite basic and lacks lots of features. You should use CMake if possible as it is way more complete (although it is still missing a few things from make).

We recently conducted a study to detect build dependency errors, focusing on missing and redundant dependencies.
*Missing dependencies (MDs), are dependencies that are not declared in the build script but will be used in the full build.
MDs prevent GNU Make from recompiling programs after they have been modified and regenerating all the targets that contain them, resulting in incorrect incremental builds
*Redundant dependencies (RDs), are dependencies that are declared in the build script but will not be used in the full build.
RDs refer to static build dependencies which declare dependencies that are not the actual build dependencies of the target. RDs cause the build system to perform unnecessary incremental builds. In addition, RDs cause targets that could be executed in parallel to be executed sequentially, reducing the build efficiency.
We analyze the actual software construction process and detect dependency errors. We found some dependencies missing (i.e. externals/tinyxml2/tinyxml2.h in /checkuninitvar.o) some dependencies redundant (i.e. dependencies in checkinternal.o).

@@ -548,7 +548,7 @@ $(libcppdir)/checkstring.o: lib/checkstring.cpp lib/addoninfo.h lib/astutils.h l
$(libcppdir)/checktype.o: lib/checktype.cpp lib/addoninfo.h lib/check.h lib/checktype.h lib/config.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checktype.cpp

$(libcppdir)/checkuninitvar.o: lib/checkuninitvar.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checknullpointer.h lib/checkuninitvar.h lib/color.h lib/config.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h
$(libcppdir)/checkuninitvar.o: lib/checkuninitvar.cpp lib/addoninfo.h lib/astutils.h lib/check.h lib/checknullpointer.h lib/checkuninitvar.h lib/color.h lib/config.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h externals/tinyxml2/tinyxml2.h
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this include was required the code would not build as there is no .h files which includes tinyxml2.h so each .cpp files which needs that information needs to include this.

In the code in question the include is not necessary as the type is forward declared and it used as pointer-only.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 @Meiye-lj please handle forward declarations in your tool because it's an important way to reduce dependencies.

@@ -791,7 +791,7 @@ test/testoptions.o: test/testoptions.cpp lib/addoninfo.h lib/check.h lib/color.h
test/testother.o: test/testother.cpp externals/simplecpp/simplecpp.h lib/addoninfo.h lib/check.h lib/checkother.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/preprocessor.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h test/helpers.h
$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testother.cpp

test/testpath.o: test/testpath.cpp lib/addoninfo.h lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h test/helpers.h
test/testpath.o: test/testpath.cpp lib/addoninfo.h lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h test/helpers.h externals/simplecpp/simplecpp.h
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also appears to be unnecessary. simplecpp.h is only included by preprocessor.h which is not includes by any other .h file. It is used pointer-only so forward declaration will suffice.

@@ -650,7 +650,7 @@ $(libcppdir)/vfvalue.o: lib/vfvalue.cpp lib/config.h lib/errortypes.h lib/mathli
cli/cmdlineparser.o: cli/cmdlineparser.cpp cli/cmdlinelogger.h cli/cmdlineparser.h cli/cppcheckexecutor.h externals/tinyxml2/tinyxml2.h lib/addoninfo.h lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/timer.h lib/utils.h
$(CXX) ${INCLUDE_FOR_CLI} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ cli/cmdlineparser.cpp

cli/cppcheckexecutor.o: cli/cppcheckexecutor.cpp cli/cmdlinelogger.h cli/cmdlineparser.h cli/cppcheckexecutor.h cli/cppcheckexecutorseh.h cli/cppcheckexecutorsig.h cli/executor.h cli/filelister.h cli/processexecutor.h cli/singleexecutor.h cli/threadexecutor.h lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/checkersreport.h lib/checkunusedfunctions.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/pathmatch.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h
cli/cppcheckexecutor.o: cli/cppcheckexecutor.cpp cli/cmdlinelogger.h cli/cmdlineparser.h cli/cppcheckexecutor.h cli/cppcheckexecutorseh.h cli/cppcheckexecutorsig.h cli/executor.h cli/filelister.h cli/processexecutor.h cli/singleexecutor.h cli/threadexecutor.h lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/checkersreport.h lib/checkunusedfunctions.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/pathmatch.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/utils.h externals/simplecpp/simplecpp.h
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also appears to be unnecessary. simplecpp.h is only included by preprocessor.h which is not includes by any other .h file. It is used pointer-only so forward declaration will suffice.

@firewave
Copy link
Collaborator

We are using our own tool dmake which generates the dependencies based on the analysis of the includes used in the code which utilizes the code our analysis is using.

We are also include-what-you-use to make sure that we are not including unnecessary includes and are also not relying on transitive includes. As of #5540 our code should be finally clean except and we only experience a few false positives - it is possible there are still some false negatives but those should be few and those should not be related to user includes but system ones.

Based on your findings it seems there might have been some shortcomings in your study which need to be addressed.

@danmar
Copy link
Owner

danmar commented Oct 13, 2023

We are also include-what-you-use to make sure

However that only checks the source code. It does not check the Makefile dependencies as far as I know.

@Meiye-lj
I think your tool which seems to check Makefile dependencies IS interesting. But it seems more development is needed. Please share with us when it works properly. If it works well I would be happy to spread the word.. it could be a valuable complement to Cppcheck..

@danmar
Copy link
Owner

danmar commented Oct 13, 2023

I close this for now. Feel free to reopen if results has been fixed.

@danmar danmar closed this Oct 13, 2023
@Meiye-lj
Copy link
Author

Meiye-lj commented Oct 14, 2023 via email

@firewave
Copy link
Collaborator

Sorry for the late reply.

You should not be using hard-coded dependencies in a Makefile at all. You should generate the dependencies based on the source. This functionality is integrated with the GNU compilers GCC and Clang via the -MM option which utilizes their preprocessor. The result looks like this:

$ gcc -MM input.cpp
input.o: input.cpp input.h
$ clang -MM input.cpp
input.o: input.cpp input.h

An example on how to integrate it into your Makefile can be found here: https://stackoverflow.com/a/313787/532627. That would be a proper future-proof fix instead of just adjusting the hard-coded ones.

Modern build systems like meson or CMake have this integrated so you don't have to take care of it by yourself anymore (not sure about Autoconf). So there is actually no external tooling necessary to do this.

dmake is basically duplicating this functionality but it is a good case of dogfooding and making sure that our preprocessor is working correctly. It does additional things though like updating the source file lists in other project files we maintain so there is no need to update those manually when we add new files.

Our Makefile is also not leveraging built-in make rules which leads to unnecessary code which is not actually necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants