makedepend gets confused with multiple #include "filename..." in same program

[Craig A Anderson]

Test condition:

In foo.cpp:

#include "bar.hpp"
#include "test1.hpp"

In test1.cpp:

#include "bar.hpp:

In test2.cpp:

#include "bar.hpp"

In test1.hpp:

#include "bar.hpp"

In bar.hpp:

#include "caa1.hpp"
#include "caa2.hpp"

---

When makedepend is processing file test1.hpp, it does not detect that bar.hpp has already been included. It DOES however, detect the fact that #include files in bar.hpp have already been processed.

Resultant output: There are two anomolies here

1. bar.hpp appears twice in the list of dependencies for foo.o (low priority)
2. All subsequent files (to makedepend) that #include bar.hpp do NOT specify

files included by bar.hpp in their dependency list (high priority)

For example:

The makedepend command line has been supplied source filenames of foo.cpp, test1.cpp, and test2.cpp

The resultant makedepend output looks something like this:

foo.o: bar.hpp caa1.hpp caa2.hpp test1.hpp bar.hpp 
test1.o: bar.hpp
test2.o: bar.hpp

---

I am actively debugging the code to see if I can fix the bug. Any help would be appreciated. I am using MSVC (I ported the project file to VS 2003).

[Olly Betts]

This bug is in the MSVC makefiles, not the build system.

And I don't see how this rates as "critical" which is defined as "crashes, loss of data, severe memory leak", so marking as "major" instead.

The makedepend implementation which the MSVC makefiles use is very poor. Another problem is that it apparently doesn't handle "#if defined ..." (we don't seem to have a bug for that).

And worst of all, it has buffer overrun problems:

​http://xapian.org/cgi-bin/bugzilla/show_bug.cgi?id=196

I'm not at all happy that we're still pointing people to a set of makefiles which include such code, but I'm not a Windows guy, so there's not a lot I can do about it short of removing the link from the download page, which seems a bit extreme. Sadly Charlie doesn't seem to care.

I'd suggest a good short-term fix for all these problems is to find a better makedepend implementation. Something in Perl would be a good choice as we already require Perl and it rules out buffer overflow issues. Richard and I have both had a quick look, but didn't find a perl one which could just be slotted in.

The best long-term fix is to sort things out so MSVC can be used with the standard build system, so we wouldn't have to maintain two parallel build systems:

​http://xapian.org/cgi-bin/bugzilla/show_bug.cgi?id=169

[Craig A Anderson]

Fair enough...but the buffer-overflow problem does seem unrelated to this bug. The buffer overflow can be corrected with the #define (or better yet, porting to C and using the STL string class)

[Olly Betts]

I'm not saying it is related as such.

Just noting that the current makedepend implementation has multiple flaws, so replacing is probably a better option than trying to fix them all. Just fixing this one flaw would be wasted effort if we then replace this implementation to address the other flaws.

I don't see how a #define can fix the buffer overflow bug. We could enlarge the buffer if that's what you mean, but there's still the potential to overflow it - just feed it a long enough string for the path.

And sure we could port to C++ and use std::string, but there are numerous "make depend" implementations out there, so picking an existing one which isn't so buggy would almost certainly be significantly less work.

[Craig A Anderson]

I know its academic (as this utility will not be maintained), but I do wish to point out that the problem which is the subject of this bug is a "logic error" and string sizes / memory allocation notwithstanding, this is an algorithmic problem.

In my particular case, switching to a Perl version of the tool is going to require some serious refactoring of my makefiles.

That said, I'm going to "give it a go" and try to fix this bug...

[Craig A Anderson]

In file include.c, starting at line 268: There is a compound 'if' statement that checks to see if the #include file is already in the list (via strcmp) _AND_ make sure the flags do not have the INCLUDED_SYM flag set.

I don't know why the flag enters into the mix - apparently, this has to do with a #define variable affecting the inclusion of subordinate #include files.

At any rate, reducing the 'if' statement to the strcmp test only appears to fix the problem.

What's the best way to update the code (or should I just make the patch valid for my version of the software)?

Comment: The code appears to use a lot of arcane techniques (IMHO) and its not very easy to follow. Sadly, the clever design is overshadowed by the sloppy implementation and lack of comments.

[Olly Betts]

Comment: The code appears to use a lot of arcane techniques (IMHO) and its not very easy to follow. Sadly, the clever design is overshadowed by the sloppy implementation and lack of comments.

You seem to have assigned this bug to yourself, which is fine if you're working on it, but that does mean that none of the Xapian developers have seen more recent updates to it as we don't currently have a mailing list for bug update messages. I happened to notice this bug while checking through bug reports while preparing a release. The state shouldn't be "RESOLVED FIXED" though, as this bug is still present in the makedepend in our tree.

In case it's not clear, we didn't write this makedepend utility - it's a piece of third party code found on the internet. I haven't studied most of the code in detail, so you probably have more idea than me how it works judging from the detail in your questions. You could try asking the original author(s) if you need clarification. If you're confident you have a suitable fix, Charlie can actually test it.

I certainly do agree that this code it has its problems - hence my belief that it's going to be simpler to replace it with an alternative implementation than to try to fix them all...

[Olly Betts]

(Description wiki-formatted)

caa: Did you come up with a suitable patch for Charlie to try?

[Olly Betts]

I had a quick look at the code - INCLUDE_SYM appears to be set for the case of:

#include SOMEHEADER

Where SOMEHEADER is a macro, set with something like:

#define SOMEHEADER "foo.h"

So I don't see why the proposed change would help, though I noticed someone else has made the same change to what appears to be an older version of this code:

​http://www.google.com/codesearch/p?hl=en#Gjh2fabJIy0/mk-3.18.4.tar.gz|dd_Gc3SlGTs/mk-3.18.4/utl/mkdep/include.c&q=%22if%20%28%28strcmp%28ip-%3Ei_incstring,%20include%29%20==%200%29%22

Charlie: are you able to reproduce this issue? If so, does the suggested fix described in comment:6 help?

[Charlie Hull]

Turns out there is a 'generate dependencies' switch for modern MSVC, '-showIncludes'. I've attached a patch to replace makedepend with 'xapdep', which parses the output of this and turns it into dependency information we can use.

This is a rather big patch I'm afraid as it affects the whole build system. Also I'm not sure the files in xapian-maintainer-tools/win32msvc/xapdep have made it into the patch, as TortoiseSVN was telling me something about subfolders, so I've included these files in a second patch file (xapdep_sub).

I'm happy to backport this to trunk if we think it's worth it.

[Charlie Hull]

Patch makefiles to use xapdep not makedepend

[Charlie Hull]

Separate patch to remove makedepend (couldn't combine this due to size limit in Trac)

[Charlie Hull]

OK, it seems that Trac doesn't like huge patches, so I've split them up. Also I can't create another patch to add xapdep sources until xapdep/ exists, so once it does let me know and I'll submit another patch.

[Charlie Hull]

Just to note, I'm going to go ahead and create build files for 1.0.18 using the old makedepend method as it's been a while since it was released. We have the xapdep method for the future though.

[Olly Betts]

Sorry for taking a while to get to this.

I've applied the patch to trunk as r14132 (and also nuked the makedepend subdirectory). I didn't commit the .sln and .vcproj files, as they don't seem to be used - xapdep.mak does the building here. If they are needed let me know.

I've eliminated 3 of the fixed sized buffers, two of which I'm pretty sure could be overflowed by suitable inputs. Paul Rubin fixed makedepend's buffer overflows, and it would be a pity to replace it with something with new ones. I'm only able to test compile with GCC, not to run, so I applied each small change separately so you can test them one by one if they don't work. Sorry if I managed to break something.

So:

Fixed a potential buffer overflow in r14133.

Eliminated another fixed sized buffer we could overflow in r14134.

And eliminated depbuf in r14135 - I don't think we could overflow this one, but it's more obvious we can't if we don't have it!

I also fixed a case of O(n*n) behaviour in r14136. Probably not a big problem given the typical line length here, but silly to be needlessly slower like that.

And finally I undoubled the contents of xapdep/README in r14137 (it and xapdep/xapdep.c were in your patch twice for some reason, but I only spotted the issue for xapdep.c before committing).

One last issue is that the "myrename" function is taken with just small modifications from the makedepend code, but this isn't reflected in the (C) statements and licensing. But it looks to me like this is just a clumsy workaround for not being able to rename a file over an existing one. Assuming there aren't other issues involved, it would be cleaner and simpler to just remove the backup file explicitly first, like this:

(void)unlink(BACKUP_MAKEFILE);
if(rename(MAKEFILE,BACKUP_MAKEFILE)!=0)
    return -1;

[Charlie Hull]

Thanks for fixing up my code, Olly, my C++ is a bit rusty. I've attached a further patch, which makes Xapdep behave a bit more like makedepend and removes myrename. I've tested it better this time as well, and it appears to work just as well as makedepend did (i.e. it's not perfect but it's better than nothing!).

[Charlie Hull]

Further patch to fix small issues with xapdep

[Charlie Hull]

oh one further thing, please put the the .sln and .vcproj files back if you can, this will mean the project can be opened in the IDE if necessary.

[Olly Betts]

In normal make it should make no difference whether the prerequisites are one per rule or many per rule, but nmake does sometimes seem to be oddly different. And splitting them should avoid any issues with limited line length.

Applied the patch with a couple of tweaks in r14148.

I avoided appending a string to objfile which might overflow it, and instead just write it to the output each time in the loop below.

GCC warns about a construct in your copying loop - I think it is undefined behaviour because ch on the LHS could be evaluated before, after, or concurrently with ch++ on the right (isn't C lovely?):

objfile[ch] = buf[ch++];

So I've tweaked that to be a for loop with the increment separate.

I'm reluctant to commit unused files as they'll bloat every checkout a bit, and sooner or later will end up out of sync. Let's just decree the nmake file as the supported way to build. If anyone really wants a project file they can create one easily enough.

So unless I broke something, this is ready for backporting.

[Charlie Hull]

remove duplicated half of .mak file

[Charlie Hull]

Looks good to me, apart from Xapdep's .mak file is duplicated for some reason - I've attached a new patch, but it should be pretty obvious to fix. I'll backport now.

[Richard Boulton]

I've removed the duplicated contents of xapdep.mak

[Olly Betts]

The duplication happened because xapdep.patch included the changes to create xapdep/xapdep.mak twice, and I failed to spot that, and patch handles this by doubling the contents. I spotted this at the time for xapdep/xapdep.c, and spotted it later for xapdep/README.

So, definitely ready to backport now.

[Olly Betts]

Backported to 1.0 as r14205 (untested).

[Charlie Hull]

Minor fix to the tests build file

[Charlie Hull]

The backport works once the above patch has been applied.

I'm still not 100% sure the method for dependency checking is working how it should though, so expect further patches at some point!

[Olly Betts]

OK, applied as r14210.

Marking as fixed.