Page MenuHomePhabricator

Merge libc++abi's lit configuration with libc++'s
ClosedPublic

Authored by jroelofs on Jan 14 2015, 5:03 PM.

Details

Reviewers
danalbert
EricWF
Summary

This is WIP, it doesn't quite work yet. On my dawin machine, I think it is picking up the system libc++abi, rather than the just-built one (maybe I've got the default backwards? not sure).

@danalbert, what do you think? Am I missing anything big?

Diff Detail

Event Timeline

jroelofs updated this revision to Diff 18197.Jan 14 2015, 5:03 PM
jroelofs retitled this revision from to Merge libc++abi's lit configuration with libc++'s.
jroelofs updated this object.
jroelofs edited the test plan for this revision. (Show Details)
jroelofs added reviewers: danalbert, EricWF.
jroelofs added a subscriber: danalbert.

Also, the diff lies... not sure why. It is showing that I deleted all these files, but in fact I've "svn mv"-ed them.

danalbert edited edge metadata.Jan 14 2015, 5:59 PM

Should improve test times significantly, since you seem to have deleted all of them ;)

I'm assuming you renamed them with .pass.cpp and forgot to add the new names (git mv is your friend). As I said in IRC, I don't really like building metadata in to the test name. I'd rather see // XCOMPILE-FAIL or similar added to the test runner instead.

Looks much better. I'm especially liking all the red. I'll try cherry-picking this tomorrow to see if it meets all my needs, but it looks like it should.

libcxxabi/test/config.py
24

os.path.join

28

I thought obj_root was supposed to be the build output directory.

31

?

36

os.path.join (and anywhere else you've done this)

56

?

lit.cfg
56
config_module = importlib.import_module(config_module_name)

I would have done that when I wrote it the first time but I had thought we bumped the minimum version to 2.6, not 2.7.

190

Did I see a patch to libc++ that added this to libcxx.test.config.Configuration earlier? I assume that's why this isn't needed any more.

lit.site.cfg.in
8

Is this actually used for anything? If it is it's in the libc++ side of this, but I can't think of what we'd actually be using it for...

I svn mv'd all the files to their *.pass.cpp equivalents. Not sure why they aren't showing up in the diff.

I could go either way on the metadata thing. Or do both, and have checks that the extension matches the metadata. Don't really have a preference.

libcxxabi/test/config.py
28

Copy-pasta from the libc++ config. That's just a default for when it can't find the right thing from cmake. I'm not sure what else to default it to.

31

That was something @EricWF added to turn off measuring the durations of things like the demangler tests.

56

The guard is named differently between libc++abi and libc++.

lit.cfg
190

Yeah.

lit.site.cfg.in
8

You're right, enable_exceptions, enable_rtti, and enable_shared aren't useful for libc++abi. Good catch.

jroelofs updated this revision to Diff 18232.Jan 15 2015, 8:54 AM
jroelofs edited edge metadata.

Address most of @danalbert's comments. I'm still not sure what I'm doing wrong with the library paths.... this still breaks tests on darwin.

jroelofs added inline comments.Jan 15 2015, 8:58 AM
CMakeLists.txt
47 ↗(On Diff #18232)

Diffs in this file bring libc++abi's version of this file most of the way in sync with libc++'s. I'm not too familiar with cmake to know whether 'add_custom_target' vs 'add_lit_testsuite' is better here. I suspect the change should be going the other way?

libcxxabi/test/config.py
23

this seems like a reasonable default now. I should delete this comment.

danalbert edited subscribers, added: Unknown Object (MLST); removed: danalbert.Jan 15 2015, 10:25 AM

Pinging in the hope that it will reach cfe-commits.

jroelofs updated this revision to Diff 18275.Jan 15 2015, 4:27 PM

Fix up the library path stuff. This at least works on darwin now. Haven't checked on linux yet.

EricWF edited edge metadata.Jan 16 2015, 12:34 PM

I can't patch CMakeLists.txt completely, Can you make sure that you diffed against the trunk?

I suspect that's because of @danalbert's recent cmake changes for cross compiling... lame.

jroelofs updated this revision to Diff 18533.Jan 21 2015, 10:08 AM
jroelofs updated this object.
jroelofs edited edge metadata.

Rebase, and add detection/configuration bits that make the libcxx imports work. Symlinking test/libcxx is no longer necessary.

Tests work on darwin now. Still haven't checked on linux.

I think I ought to separate the "rename all the files" change from this.

Here's a separate patch that does the file renaming: http://reviews.llvm.org/D7101

jroelofs updated this revision to Diff 18542.Jan 21 2015, 11:21 AM

Rebase after D7101

danalbert added inline comments.Jan 21 2015, 2:42 PM
CMakeLists.txt
47 ↗(On Diff #18542)

What prevents us from continuing to use add_lit_testsuite? If I'm reading the definition correctly, the function will automatically use LLVM_LIT_ARGS (used indirectly in add_lit_target). That also keeps the FindPythonInterp` stuff delegated to the LLVM cmake code.

libcxxabi/test/config.py
44

os.path.join (The leading slash isn't necessary with os.path.join. I'd like to believe Windows will handle that gracefully, but I don't actually know.)

I could have merged that either way, actually. I wasn't sure which one was more preferable.

danalbert accepted this revision.Jan 21 2015, 3:01 PM
danalbert edited edge metadata.

Reusing the LLVM cmake code is preferred. Once that is fixed up, LGTM.

This revision is now accepted and ready to land.Jan 21 2015, 3:01 PM
jroelofs updated this revision to Diff 18560.Jan 21 2015, 3:05 PM
jroelofs edited edge metadata.

Merge the CMakeLists.txt the other way.

jroelofs closed this revision.Jan 22 2015, 1:04 PM

Committed in r226737, (and buildbots appeased in r226749)