This is an archive of the discontinued LLVM Phabricator instance.

Use test-specific module caches to avoid stale header conflicts
ClosedPublic

Authored by vsk on Jan 18 2018, 7:03 PM.

Details

Summary

Stale global module caches cause problems for the bots. The modules
become invalid when clang headers are updated by version control, and
tests which use these modules fail to compile, e.g:

fatal error: file '.../__stddef_max_align_t.h' has been modified since the module file '/var/.../Darwin.pcm' was built
note: please rebuild precompiled header '/var/.../Darwin.pcm'

Eventually we should transition to having just a single module cache to speed
tests up. This patch should be just enough to fix the spurious bot failures due
to stale caches.

rdar://36479805

Diff Detail

Event Timeline

vsk created this revision.Jan 18 2018, 7:03 PM

Thanks, this is a great start!

packages/Python/lldbsuite/test/make/Makefile.rules
240

Is it safe that this is a relative path?

242

Should we also specify -fcxxmodules here? Or will that break some bots with older clangs?

vsk updated this revision to Diff 130566.Jan 19 2018, 12:56 AM
  • Upload a diff with context.
packages/Python/lldbsuite/test/make/Makefile.rules
240

I admit it's not great, but it's at least in keeping with the other targets we define in this Makefile (like a.out, the dsym, etc). As I'm writing this, I've realized I did not upload a diff with context, so this might not have been apparent!

242

I've tried to avoid changing the flags used by each Makefile when possible. The only exception is my removal of "$(subst -fmodules,, $(CFLAGS))" in CXXFLAGS, which I needed to do because the substitution would make us pass "-cache-path=foo".

We might consider adding in -fcxxmodules as a followup (maybe just in CXXFLAGS?).

I don't know much about clang modules, but the change seems reasonable to me.

vsk updated this revision to Diff 130686.Jan 19 2018, 1:51 PM

Per an offline comment by Adrian, include -gmodules in the mandatory set of module flags.

vsk updated this revision to Diff 131000.Jan 22 2018, 7:26 PM

Skip tests which fail when -fmodules is passed (https://bugs.llvm.org/show_bug.cgi?id=36048).

Wait.. this patch is not supposed to change the set of tests that get -fmodules passed to them. It should only add -fmodules-cache-path to tests that do pass -fmodules. Why is this necessary?

vsk added a comment.Jan 23 2018, 10:51 AM

Skip tests which fail when -fmodules is passed (https://bugs.llvm.org/show_bug.cgi?id=36048).

Wait.. this patch is not supposed to change the set of tests that get -fmodules passed to them. It should only add -fmodules-cache-path to tests that do pass -fmodules. Why is this necessary?

It's not necessary, but we should make this change. It falls out of removing this FIXME:

# FIXME: C++ modules aren't supported on all platforms.
CXXFLAGS += $(subst -fmodules,, $(CFLAGS))

If we keep it around, we'd need an extra hack to strip out "-fmodules-cache-path=module-cache" as well. Without that we'd pass "-cache-path=module-cache", which clang would reject.

Another benefit of making this change is that it lets us know which tests are actually failing with modules enabled, instead of hiding them.

Okay got it. I'll investigate the PR separately.

vsk added a comment.Jan 24 2018, 7:09 PM

Is this OK to commit?

aprantl accepted this revision.Jan 25 2018, 9:17 AM
This revision is now accepted and ready to land.Jan 25 2018, 9:17 AM
This revision was automatically updated to reflect the committed changes.