This is an archive of the discontinued LLVM Phabricator instance.

[lit] Implement `not` as a builtin in the Lit internal shell
AbandonedPublic

Authored by ldionne on Oct 15 2020, 12:50 PM.

Details

Summary

Instead of using LLVM's not.cpp, implement not in the internal Lit
shell as a builtin. This allows creating test suites that use the
internal Lit shell without depending on llvm/utils/not, which in
turn depends on the LLVM support libraries.

This work is done in the context of trying to move the libc++ test
suite to the internal Lit shell -- the libc++ test suite currently
doesn't depend on the LLVM support libraries, and it would be ideal
to keep it that way.

Diff Detail

Event Timeline

ldionne created this revision.Oct 15 2020, 12:50 PM
ldionne requested review of this revision.Oct 15 2020, 12:50 PM

I'm not entirely satisfied with this patch -- if folks think we should implement not as an in-process builtin, let me know and I can look into that. It's also annoying that we're treating not differently from the other out-of-process builtins after this patch, but I couldn't find another way to do it.

I'm not entirely satisfied with this patch -- if folks think we should implement not as an in-process builtin, let me know and I can look into that.

I guess the advantage here would be avoiding spawning another process? A downside might mean it's harder to test the builtin implementation unless we write the implementation to be testable. I hadn't looked at the builtin code in lit before. It's interesting that it already tries to parse the --crash arg to not.

Shouldn't we have some tests to test your implementation of the not builtin?

llvm/utils/lit/lit/builtin_commands/not.py
65

You might want to use sys.exit() here instead of exit(). exit() is meant for use in interactive shells.

delcypher added inline comments.Oct 15 2020, 10:52 PM
llvm/utils/lit/lit/TestRunner.py
657

Nit: I don't like having args.pop(0) + ".py" here. I think it makes the code harder to read due to args.pop(0) both returning a value but also having a side-effect.

llvm/utils/lit/lit/builtin_commands/not.py
34

Are you missing a \n here?

ldionne marked 3 inline comments as done.Oct 16 2020, 6:02 AM

I'm not entirely satisfied with this patch -- if folks think we should implement not as an in-process builtin, let me know and I can look into that.

I guess the advantage here would be avoiding spawning another process? A downside might mean it's harder to test the builtin implementation unless we write the implementation to be testable. I hadn't looked at the builtin code in lit before. It's interesting that it already tries to parse the --crash arg to not.

Shouldn't we have some tests to test your implementation of the not builtin?

The implementation is already tested -- the shtest-not tests are running in the internal shell, and are testing that very implementation. As it stands, it mostly means that the original not.cpp in LLVM's utilities is the thing that's not tested anymore, I think.

ldionne updated this revision to Diff 298611.Oct 16 2020, 6:03 AM

Address Dan's comments

ldionne updated this revision to Diff 306759.Nov 20 2020, 12:18 PM

Rebase onto master.

ldionne updated this revision to Diff 340161.Apr 23 2021, 1:55 PM

Rebase onto main

LGTM other than my nits.

I'm adding @yln as reviewer just in case he has thoughts on this change.

llvm/utils/lit/lit/builtin_commands/not.py
16

Minor nit: echo probably isn't a good example here as it's usually a shell builtin so it's not something that this script can actually invoke.

33

Do we need to call sys.stderr.flush() here? I recently ran into a problem with sys.stdout where I needed to do this to ensure it gets written out but I'm not sure about stderr.

dexonsmith added inline comments.Apr 24 2021, 11:27 AM
llvm/utils/lit/lit/builtin_commands/not.py
33

I don't know for sure about python, but usually stdout is buffered and stderr isn't.