[PATCH] D31236: Refactor getHostCPUName to allow testing on non-native hardware.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

In https://reviews.llvm.org/D31236#707462, @hfinkel wrote:

> I'm curious why you chose to take this approach rather than add some option that allows us to change the file name being read? If we do that, then we can test this with lit tests. I generally think of our practice as using mocking, and unit tests in general, for cases where lit tests aren't practical (or, to put it another way, the infrastructure necessary to enable them is more complicated than unit testing in C++). This does not seem to be the case here. It seems straightforward to make -proc-cpuinfo-file=/foo/bar/cpuinfo.txt (modulo bikeshed) work.
>
> As I recall, there are several other places in Clang where this is also a problem (we have hard-coded file names for /etc/lsb-release, /etc/redhat-release, etc.).


I honestly hadn't thought this could fit in our lit testing framework. But maybe it could be made to do so as you outline above.
I see that tools/clang/unittests/Driver/DistroTest.cpp uses unittests with vfs::InMemoryFileSystem objects to mock the contents of /etc/lsb-release etc. I wasn't aware of the approach taken there, I'll take a closer look.

I think the main issue with a -proc-cpuinfo-file=%s command line option might be in which tool to attach it to. I guess it would have to be llc, run with -mcpu=native, and then somehow detecting the cpu it would set for code generation.

I think that might work for current functionality (returning a single CPU), but when we're trying to extend this to big.LITTLE systems, and introducing a call like getHostCPUNames(), returning all different cores in the system, I'm not sure anymore if it would be easily tested using "llc -mcpu=native", as it's unclear which core to pick for "native". FWIW, I expect getHostCPUNames() to initially mainly be used by JIT engines and they may make different choices than ahead-of-time compilers with -mcpu=native.

I'll look into this a bit further, but at the moment my feel is that testing via llc -mcpu=native may be too indirect for e.g. extending this API to big.LITTLE systems. I'm not sure if there is another tool already where we could test closer to getHostCPUName, but I don't think so.


https://reviews.llvm.org/D31236