[Lldb-commits] [PATCH] D61611: [JITLoaderGDB] Set eTypeJIT for objects read from JIT descriptors

Thu May 9 09:09:23 PDT 2019

stella.stamenova added a comment.

In D61611#1496681 <https://reviews.llvm.org/D61611#1496681>, @sgraenitz wrote:

> In D61611#1496644 <https://reviews.llvm.org/D61611#1496644>, @probinson wrote:
>
> > Sorry for the drive-by... what is this `REQUIRES: nowindows`?  I don't see where lit generates this property.  I grepped all of llvm-project.git and I see it used in two tests, but not where it's produced. Which suggests that those tests don't actually run *anywhere*.
>
>
> Thanks for the heads-up. I also just grepped. The history says that Stella will know more about it. I went with `XFAIL` for now.

The syntax is supposed to be different: XFAIL means that the test is expected to fail no that platform, but it will still run. This makes sense for tests that should eventually pass on that platform, but don't yet. REQUIRES: noX means that the test won't run unless the requirement is satisfied. In the case of REQUIRES: nowindows this means that the test does not even support windows and it should never pass there. There are currently a couple of tests in LLVM and LLDB marked with REQUIRES: nowindows because they are not expected to run there at all.

The "feature" nowindows is supposed to be added to the available features in lit/llvm/config.py, but it looks like it's not at the moment, so I am wondering if the two tests that are marked as nowindows run on any platform right now. I'll update config.py to create the right features, so you can assume that nosystem-windows will be a "feature" that is set on non-windows platforms (since we use system-X now).

For your tests, I think they are expected to *eventually* pass on Windows even if they don't today, correct? If that's the case, then you should use XFAIL. Otherwise, you should use REQUIRES: nosystem-windows.

I can apply your patch locally tomorrowish and run the tests to see how they behave on Windows.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61611/new/

https://reviews.llvm.org/D61611