This is an archive of the discontinued LLVM Phabricator instance.

Differential D42277

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

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

Details

Reviewers
aprantl
davide
jasonmolenda
labath
Commits
rG801362f067ae: Use test-specific module caches to avoid stale header conflicts
rLLDB323450: Use test-specific module caches to avoid stale header conflicts
rL323450: Use test-specific module caches to avoid stale header conflicts
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
Herald added a subscriber: eraman. · View Herald TranscriptJan 18 2018, 7:03 PM
aprantl added a comment.Jan 18 2018, 7:36 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?).

labath added a comment.Jan 19 2018, 2:59 AM

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
aprantl added a comment.Jan 23 2018, 6:31 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?

vsk added a comment.Jan 23 2018, 10:51 AM
In D42277#985002, @aprantl wrote:

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.

aprantl added a comment.Jan 23 2018, 11:18 AM

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
Closed by commit rL323450: Use test-specific module caches to avoid stale header conflicts (authored by vedantk). · Explain WhyJan 25 2018, 10:02 AM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: llvm-commits. · View Herald TranscriptJan 25 2018, 10:02 AM

Revision Contents

PathSize
packages/
Python/
lldbsuite/
test/
lang/
objc/
modules-auto-import/
2 lines
modules-incomplete/
2 lines
modules-inline-functions/
2 lines
make/
12 lines

Diff 130538

packages/Python/lldbsuite/test/lang/objc/modules-auto-import/Makefile

LEVEL = ../../../make LEVEL = ../../../make
OBJC_SOURCES := main.m OBJC_SOURCES := main.m
CFLAGS += -fmodules -gmodules -g CFLAGS += -gmodules $(MANDATORY_MODULE_BUILD_CFLAGS)
include $(LEVEL)/Makefile.rules include $(LEVEL)/Makefile.rules

packages/Python/lldbsuite/test/lang/objc/modules-incomplete/Makefile

Context not available.
include $(LEVEL)/Makefile.rules include $(LEVEL)/Makefile.rules
CFLAGS += -fmodules -I$(PWD) CFLAGS += $(MANDATORY_MODULE_BUILD_CFLAGS) -I$(PWD)
LDFLAGS += -framework Foundation LDFLAGS += -framework Foundation
Context not available.

packages/Python/lldbsuite/test/lang/objc/modules-inline-functions/Makefile

Context not available.
include $(LEVEL)/Makefile.rules include $(LEVEL)/Makefile.rules
CFLAGS += -fmodules -I$(PWD) CFLAGS += $(MANDATORY_MODULE_BUILD_CFLAGS) -I$(PWD)
Context not available.

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

Context not available.
CFLAGS += -gsplit-dwarf CFLAGS += -gsplit-dwarf
endif endif
CLANG_MODULE_CACHE_DIR := module-cache
aprantlUnsubmitted
Not Done
ReplyInline Actions

Is it safe that this is a relative path?

aprantl: Is it safe that this is a relative path?
vskAuthorUnsubmitted
Not Done
ReplyInline Actions

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!

vsk: I admit it's not great, but it's at least in keeping with the other targets we define in this…
MANDATORY_MODULE_BUILD_CFLAGS := -fmodules -fmodules-cache-path=$(CLANG_MODULE_CACHE_DIR)
aprantlUnsubmitted
Not Done
ReplyInline Actions

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

aprantl: Should we also specify `-fcxxmodules` here? Or will that break some bots with older clangs?
vskAuthorUnsubmitted
Not Done
ReplyInline Actions

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?).

vsk: I've tried to avoid changing the flags used by each Makefile when possible. The only exception…
ifeq "$(MAKE_GMODULES)" "YES" ifeq "$(MAKE_GMODULES)" "YES"
CFLAGS += -fmodules -gmodules CFLAGS += -gmodules $(MANDATORY_MODULE_BUILD_CFLAGS)
endif endif
CXXFLAGS += -std=c++11 CXXFLAGS += -std=c++11 $(CFLAGS)
# FIXME: C++ modules aren't supported on all platforms.
CXXFLAGS += $(subst -fmodules,, $(CFLAGS))
LD = $(CC) LD = $(CC)
LDFLAGS ?= $(CFLAGS) LDFLAGS ?= $(CFLAGS)
LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS) LDFLAGS += $(LD_EXTRAS) $(ARCH_LDFLAGS)
Context not available.
dsym: $(DSYM) dsym: $(DSYM)
all: $(EXE) $(DSYM) all: $(EXE) $(DSYM)
clean:: clean::
$(RM) $(OBJECTS) $(PREREQS) $(PREREQS:.d=.d.tmp) $(DWOS) $(ARCHIVE_NAME) $(ARCHIVE_OBJECTS) $(RM) -rf $(OBJECTS) $(PREREQS) $(PREREQS:.d=.d.tmp) $(DWOS) $(ARCHIVE_NAME) $(ARCHIVE_OBJECTS) $(CLANG_MODULE_CACHE_DIR)
ifneq "$(DYLIB_NAME)" "" ifneq "$(DYLIB_NAME)" ""
$(RM) -r $(DYLIB_FILENAME).dSYM $(RM) -r $(DYLIB_FILENAME).dSYM
$(RM) $(DYLIB_OBJECTS) $(DYLIB_PREREQS) $(DYLIB_PREREQS:.d=.d.tmp) $(DYLIB_DWOS) $(DYLIB_FILENAME) $(DYLIB_FILENAME).debug $(RM) $(DYLIB_OBJECTS) $(DYLIB_PREREQS) $(DYLIB_PREREQS:.d=.d.tmp) $(DYLIB_DWOS) $(DYLIB_FILENAME) $(DYLIB_FILENAME).debug
Context not available.