This is an archive of the discontinued LLVM Phabricator instance.

[lit] Remove TODO
ClosedPublic

Authored by modocache on Oct 11 2016, 9:40 PM.

Event Timeline

modocache updated this revision to Diff 74320.Oct 11 2016, 9:40 PM
modocache retitled this revision from to [lit] Remove TODO.
modocache updated this object.
modocache added a subscriber: llvm-commits.
modocache updated this revision to Diff 74471.Oct 12 2016, 7:34 PM

Update the README

delcypher edited edge metadata.Oct 19 2016, 3:42 AM

@modocache Sorry for the delay in taking a look at this.

This change mostly LGTM except it looks like you've dropped these TODOs

* Move temp directory name into local test config.

* Support valgrind in all configs, and LLVM style valgrind.

* Support ulimit.

* Create an explicit test suite object (instead of using the top-level TestingConfig object).

Is that intentional?

As for the others:

  • I chose not to migrate "Create an explicit test suite object (instead of using the top-level TestingConfig object)" because I wasn't sure it was still relevant. It also seems like an implementation detail to me.
  • I'd appreciate it if you could create a problem report for "Support valgrind in all configs, and LLVM style valgrind", @delcypher! I'm not sure I understand what is and isn't supported.
echristo accepted this revision.Nov 1 2016, 10:29 AM
echristo edited edge metadata.
This revision is now accepted and ready to land.Nov 1 2016, 10:29 AM

@modocache

I'd appreciate it if you could create a problem report for "Support valgrind in all configs, and LLVM style valgrind", @delcypher! I'm not sure I understand what is and isn't supported.

I'm not entirely sure what this is either. lit can run tools under valgrind but I'm not sure if valgrind is actively used much these days for LLVM development. There are valgrind suppression files in the LLVM source tree (utils/valgrind) but they haven't been touched in a while but I'm guessing that's what this TODO was referring to. Really the idea of running the tests with a supervising tool needs to be generalized so arbitary stuff can be plugged in. For example it might be useful to have the system debugger attached to every process spawned so if one of them crashes it's easy to see what happened.

I could create a problem report for that if you like.

Also sorry for my slowness in replying :(

delcypher accepted this revision.Nov 3 2016, 9:58 AM
delcypher edited edge metadata.
modocache closed this revision.Nov 3 2016, 4:51 PM

Thanks @delcypher! No worries about any lateness, I really appreciate the review!

A problem report for Valgrind would be appreciated, even if it's just "figure out what that old TODO was referring to." :)