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

memcached branch corrupts dependency file #352

Closed
gjasny opened this issue Jan 29, 2019 · 11 comments
Closed

memcached branch corrupts dependency file #352

gjasny opened this issue Jan 29, 2019 · 11 comments
Labels
bug Does not work as intended/documented closed: won't fix/implement/merge Will not fix/implement/merge

Comments

@gjasny
Copy link
Contributor

gjasny commented Jan 29, 2019

Hello,

The following testcase reproduces the creation of a mismatching .d file:

#!/bin/bash

#set -x
set -e

export PROJECT_ROOT="${PWD}"
export CCACHE_DIR="${PROJECT_ROOT}/cache"
export CCACHE_LOGFILE="${PROJECT_ROOT}/cache.log"
export CCACHE_MEMCACHED_ONLY=true
export CCACHE_MEMCACHED_CONF="--SERVER=localhost"
export CCACHE_NOHASHDIR=true

: ${CCACHE:=ccache}
: ${CXX:=c++}

echo -e 'flush_all\nquit\n' | nc localhost 11211

rm -rf "${CCACHE_DIR}" "${CCACHE_LOGFILE}" s1 s2
mkdir -p "${CCACHE_DIR}" s1

cat << EOF > s1/foo.h
#pragma once
int foo();
EOF

cp -a s1 s2

cat << EOF > s1/foo.cpp
#include "${PROJECT_ROOT}/s1/foo.h"

int foo() { return 42; }
EOF

cat << EOF > s2/foo.cpp
#include "${PROJECT_ROOT}/s2/foo.h"

int foo() { return 42; }
EOF

sleep 2

cd s1 
export CCACHE_BASEDIR="${PWD}"
"${CCACHE}" "${CXX}" -c "${PWD}/foo.cpp" -o "${PWD}/foo.o" -MD -MF "${PWD}/foo.o.d"
echo "--- s1"
cat "${PWD}/foo.o.d"
cd ..

cd s2
export CCACHE_BASEDIR="${PWD}"
"${CCACHE}" "${CXX}" -c "${PWD}/foo.cpp" -o "${PWD}/foo.o" -MD -MF "${PWD}/foo.o.d"
echo "--- s2 first"
cat "${PWD}/foo.o.d"
cd ..


rm -rf "${CCACHE_DIR}" s2
mkdir -p "${CCACHE_DIR}" s2

cat << EOF > s2/foo.h
#pragma once
int foo();
EOF

cat << EOF > s2/foo.cpp
#include "${PROJECT_ROOT}/s2/foo.h"

int foo() { return 42; }
EOF

cd s2
export CCACHE_BASEDIR="${PWD}"
"${CCACHE}" "${CXX}" -c "${PWD}/foo.cpp" -o "${PWD}/foo.o" -MD -MF "${PWD}/foo.o.d"
echo "--- s2 second"
cat "${PWD}/foo.o.d"
cd ..

#"${CCACHE}" --show-stats

When I run it on Ubuntu 18.10 with dev/memcached head (3.4.3+136_gf771208) I get the following output:

OK
--- s1
foo.o: foo.cpp /usr/include/stdc-predef.h \
 /home/gregorj/Git/Experimental/ccache-deps-bug/s1/foo.h
--- s2 first
foo.o: foo.cpp /usr/include/stdc-predef.h \
 /home/gregorj/Git/Experimental/ccache-deps-bug/s2/foo.h
--- s2 second
foo.o: foo.cpp /usr/include/stdc-predef.h \
 /home/gregorj/Git/Experimental/ccache-deps-bug/s1/foo.h

Please note that the s2/foo.o.d file contains /home/gregorj/Git/Experimental/ccache-deps-bug/s1/foo.h.

@afbjorklund That only happens on the memcached branch.

Thanks,
Gregor

@jrosdahl
Copy link
Member

jrosdahl commented Feb 3, 2019

Thank you for reporting this bug. The dev/memcached branch is however not maintained as described in this and this comment to issue #58.

That said, I think that the bug is because there is a missing call to use_relative_paths_in_depfile in to_memcached. This is actually a good illustration of why the solution on the dev/memcached branch needs to be revised – logic like this should not be duplicated in memcached-specific code.

@jrosdahl jrosdahl added bug Does not work as intended/documented closed: won't fix/implement/merge Will not fix/implement/merge labels Feb 3, 2019
@jrosdahl jrosdahl closed this as completed Feb 3, 2019
@afbjorklund
Copy link
Contributor

Or alternatively, it should be fixed in both places.

@jrosdahl
Copy link
Member

jrosdahl commented Feb 3, 2019

Or alternatively, it should be fixed in both places.

Sorry, what do you mean?

@afbjorklund
Copy link
Contributor

Sometimes things have to be duplicated (e.g. master and maint), which means fixes have to be too.

@jrosdahl
Copy link
Member

jrosdahl commented Feb 3, 2019

Sometimes things have to be duplicated (e.g. master and maint), which means fixes have to be too.

I still don't follow. There is only one place where a fix could be applied in this case. What do you think should be done?

@afbjorklund
Copy link
Contributor

Never mind, just that we need a better plan for dev/memcached than unmaintained

@jrosdahl
Copy link
Member

jrosdahl commented Feb 3, 2019

OK. I'll remove the branch to make it more clear that it's unsupported.

@afbjorklund
Copy link
Contributor

Hopefully it can make a comeback in a form that can be supported ?

@gjasny
Copy link
Contributor Author

gjasny commented Feb 3, 2019

😢

@gjasny
Copy link
Contributor Author

gjasny commented Feb 3, 2019

diff --git a/src/ccache.c b/src/ccache.c
index 6a57071..e0e5a09 100644
--- a/src/ccache.c
+++ b/src/ccache.c
@@ -1647,9 +1647,12 @@ to_memcached(struct args *args)
 
        char *dep_d = NULL;
        size_t dep_l = 0;
-       if (generating_dependencies && !read_file(output_dep, 0, &dep_d, &dep_l)) {
-               stats_update(STATS_ERROR);
-               failed();
+       if (generating_dependencies) {
+               use_relative_paths_in_depfile(output_dep);
+               if (!read_file(output_dep, 0, &dep_d, &dep_l)) {
+                       stats_update(STATS_ERROR);
+                       failed();
+               }
        }
 
        if (memccached_set(cached_key, obj_d, stderr_d, dia_d, dep_d,

@afbjorklund
Copy link
Contributor

@gjasny : thanks for the patch!

I have some plans to keep the old code running against the maint branch, e.g. 3.4.x

But we need to do some major refactoring, in order to go to master branch (i.e. 3.7)

Hopefully that can work for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Does not work as intended/documented closed: won't fix/implement/merge Will not fix/implement/merge
Projects
None yet
Development

No branches or pull requests

3 participants