Fixed 47 missing dependencies and 51 excessive dependencies in Makefile #1522

vemakereporter · 2019-07-25T16:49:57Z

Hi, I've fixed 47 missing dependencies and 51 excessive dependencies reported.
Those issues can cause incorrect results or unnecessary rebuilds when capstone is incrementally built.

For example, any changes in "arch/AArch64/AArch64BaseInfo.h" will not cause "arch/AArch64/AArch64BaseInfo.o" to be rebuilt, which is incorrect.

"MCInstrDesc.h" is specified as the prerequisite of "arch/Mips/MipsDisassembler.o". However, this header file is not read by the process when "arch/Mips/MipsDisassembler.o" is built. Hence, the changes in "MCInstrDesc.h" can cause an unnecessary rebuild of "arch/Mips/MipsDisassembler.o", which slow down the build of the whole project.

A useful tool is the -M class of flags that we can pass to clang/gcc to generate "dependency files". Even if we don't use the flag directly in the Makefile, a script that we use could take advantage of the feature.
I modify the Makefile to read dependencies from compiler-generated files. I've checked. The dependences generate from the new Makefile are correct.
Looking forward to your confirmation.

Thanks
Vemake

vemakereporter · 2019-07-25T16:51:55Z

@aquynh @tmfink I have resubmitted the new PR to next track. Waiting for your confirmation.

tmfink · 2019-07-26T01:26:27Z

@vemakereporter After doing a make and make clean, there are still *.d files in tests/ and suite/fuzz/.

$ make && make clean
$ find . -type f -name '*.d'
./tests/test_mos65xx.d
./tests/test_m68k.d
./tests/test_iter.d
./tests/test_systemz.d
./tests/test_arm.d
./tests/test_mips.d
./tests/test_evm.d
./tests/test_wasm.d
./tests/test_xcore.d
./tests/test_x86.d
./tests/test_skipdata.d
./tests/test_riscv.d
./tests/test_ppc.d
./tests/test_sparc.d
./tests/test_detail.d
./tests/test_basic.d
./tests/test_m680x.d
./tests/test_bpf.d
./tests/test_tms320c64x.d
./tests/test_arm64.d
./tests/test_customized_mnem.d
./suite/fuzz/drivermc.d
./suite/fuzz/fuzz_decode_platform.d
./suite/fuzz/fuzz_disasm.d
./suite/fuzz/driverbin.d
./suite/fuzz/platform.d

tmfink · 2019-07-26T01:30:15Z

For tracking purposes, this is a follow-up to PR #1521 to the next branch.

Makefile

vemakereporter · 2019-07-26T03:47:29Z

@vemakereporter After doing a make and make clean, there are still *.d files in tests/ and suite/fuzz/.

$ make && make clean
$ find . -type f -name '*.d'
./tests/test_mos65xx.d
./tests/test_m68k.d
./tests/test_iter.d
./tests/test_systemz.d
./tests/test_arm.d
./tests/test_mips.d
./tests/test_evm.d
./tests/test_wasm.d
./tests/test_xcore.d
./tests/test_x86.d
./tests/test_skipdata.d
./tests/test_riscv.d
./tests/test_ppc.d
./tests/test_sparc.d
./tests/test_detail.d
./tests/test_basic.d
./tests/test_m680x.d
./tests/test_bpf.d
./tests/test_tms320c64x.d
./tests/test_arm64.d
./tests/test_customized_mnem.d
./suite/fuzz/drivermc.d
./suite/fuzz/fuzz_decode_platform.d
./suite/fuzz/fuzz_disasm.d
./suite/fuzz/driverbin.d
./suite/fuzz/platform.d

test and suite/fuzz have their separate Makefiles. I include a removal command in their clean targets too.

vemakereporter · 2019-07-26T04:10:36Z

@tmfink @aquynh

tmfink · 2019-07-26T05:02:32Z

suite/fuzz/Makefile


 clean:
 	rm -rf fuzz_harness $(OBJS) $(PLATFORMDECODE) $(REPRODUCERMC) $(REPRODUCERBIN) $(FUZZERBIN) $(OBJDIR)/lib$(LIBNAME).* $(OBJDIR)/$(LIBNAME).*
+	rm -rf *.d $(OBJDIR)/*.d


I don't think you want the '-r' day here

Yes, you are right

tmfink · 2019-07-26T05:02:46Z

tests/Makefile


 clean:
 	rm -rf $(OBJS) $(BINARY) $(TESTDIR)/*.exe $(TESTDIR)/*.static $(OBJDIR)/lib$(LIBNAME).* $(OBJDIR)/$(LIBNAME).*
+	rm -rf *.d $(TESTDIR)/*.d $(OBJDIR)/*.d


Same as above

Yes, you are right.

vemakereporter · 2019-07-26T07:00:18Z

@tmfink @aquynh Is it okay?

vemakereporter · 2019-07-26T09:43:33Z

@tmfink @aquynh We have tested the project again, and found that these header files cause 51 excessive dependencies in total, which cause unnecessary incremental rebuilds of 51 targets including "arch/Mips/MipsDisassembler.o". Looking forward to your confirmation.

aquynh · 2019-07-26T16:14:32Z

cstool/Makefile


 clean:
 	${RM} -rf *.o $(TARGET)
+	${RM} -rf *.d


there is no more .o files?

Yes, it just creates .d files aside to the .o files.

Makefile

vemakereporter · 2019-07-27T16:45:18Z

@tmfink @aquynh We have fixed it.

aquynh · 2019-07-29T02:37:29Z

cool, just 1 minor issue now: we have a conflict in Makefile, so i cannot merge. could you please fix it?

vemakereporter · 2019-07-29T04:21:12Z

cool, just 1 minor issue now: we have a conflict in Makefile, so i cannot merge. could you please fix it?

Conflict resolved.

aquynh · 2019-07-29T06:15:13Z

merged, thanks!

vemakereporter changed the title ~~Fixed 57 missing dependencies in Makefile~~ Fixed 47 missing dependencies and 10 excessive dependencies in Makefile Jul 25, 2019

tmfink reviewed Jul 26, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Fix Excessive and Missing Dependencies found by Vemake

f97606f

vemakereporter force-pushed the next branch from b77723c to f97606f Compare July 26, 2019 03:28

vemakereporter added 2 commits July 26, 2019 03:29

Remove extra spaces at the end of Makefile

39bf113

Remove used macro df

e1a5591

tmfink suggested changes Jul 26, 2019

View reviewed changes

vemakereporter added 2 commits July 26, 2019 14:54

Change "-rf" to "-f" in tests/Makefile

e21a651

Change "-rf" to "-f" in suite/fuzz/Makefile

f9f0b90

vemakereporter changed the title ~~Fixed 47 missing dependencies and 10 excessive dependencies in Makefile~~ Fixed 47 missing dependencies and 51 excessive dependencies in Makefile Jul 26, 2019

aquynh reviewed Jul 26, 2019

View reviewed changes

Makefile Show resolved Hide resolved

vemakereporter added 2 commits July 27, 2019 22:27

Remove 'r' from the removal command.

f9dd7b8

Remove an extra blank line.

dd1d248

tmfink approved these changes Jul 29, 2019

View reviewed changes

Merge branch 'next' into next

e7b51bf

aquynh merged commit eb35714 into capstone-engine:next Jul 29, 2019

riptl mentioned this pull request Jul 22, 2022

Add PowerPC paired-singles support #1898

Merged

6 tasks

Fixed 47 missing dependencies and 51 excessive dependencies in Makefile #1522

Fixed 47 missing dependencies and 51 excessive dependencies in Makefile #1522

Uh oh!

Conversation

vemakereporter commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vemakereporter commented Jul 25, 2019

Uh oh!

tmfink commented Jul 26, 2019

Uh oh!

tmfink commented Jul 26, 2019

Uh oh!

Uh oh!

vemakereporter commented Jul 26, 2019

Uh oh!

vemakereporter commented Jul 26, 2019

Uh oh!

tmfink Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

vemakereporter Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

tmfink Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

vemakereporter Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

vemakereporter commented Jul 26, 2019

Uh oh!

vemakereporter commented Jul 26, 2019

Uh oh!

aquynh Jul 26, 2019

Choose a reason for hiding this comment

Uh oh!

vemakereporter Jul 27, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vemakereporter commented Jul 27, 2019

Uh oh!

aquynh commented Jul 29, 2019

Uh oh!

vemakereporter commented Jul 29, 2019

Uh oh!

aquynh commented Jul 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vemakereporter commented Jul 25, 2019 •

edited

Loading