This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Use COMPILER_RT_EMULATOR with gtests
ClosedPublic

Authored by vitalybuka on Apr 21 2021, 2:20 PM.

Diff Detail

Event Timeline

vitalybuka created this revision.Apr 21 2021, 2:20 PM
vitalybuka requested review of this revision.Apr 21 2021, 2:20 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 21 2021, 2:20 PM
Herald added subscribers: llvm-commits, Restricted Project. · View Herald Transcript
This revision is now accepted and ready to land.Apr 21 2021, 4:31 PM

@vitalybuka Thanks for tackling this. I've always wanted to have our unit tests be runnable on targets that aren't the host. This looks like the first step towards that.

This revision was landed with ongoing or failed builds.Apr 22 2021, 10:40 AM
This revision was automatically updated to reflect the committed changes.

The approach looks good but I'd like to see us type check the new run_under parameter.

llvm/utils/lit/lit/formats/googletest.py
25

It looks like we require self.run_under to be a list but we never check that it is one. We should check the type here and fail if it's wrong with litConfig.fatal_error().

It looks like

if emulator:
  emulator = shlex.split(emulator)

currently ensures we pass in a list but this is not very obvious because at first I thought we were passing in a string.

158

If self.run_under isn't a list then it'll cause problems here.

@vitalybuka Damn looks like you landed the patch while I was in the middle of writing a review. Could you make the fix I requested as a follow up patch?

@vitalybuka Damn looks like you landed the patch while I was in the middle of writing a review. Could you make the fix I requested as a follow up patch?

sure, sorry, I didn't noticed comments.
I even can revert if there is serious issues.

@vitalybuka Damn looks like you landed the patch while I was in the middle of writing a review. Could you make the fix I requested as a follow up patch?

sure, sorry, I didn't noticed comments.
I even can revert if there is serious issues.

@vitalybuka No problem. No need to revert. I don't think the patch will break anything that exists today but I would like to make sure the new run_under parameter gets typed checked so that it's easier for people to adopt it. Please create a follow up patch and add myself and @yln as reviewers.

vitalybuka reopened this revision.Apr 22 2021, 11:20 AM
vitalybuka added 2 blocking reviewer(s): delcypher, yln.

I reverted as I'd like also to see which bots it brakes, but it landed with another commit which seriously broke clang compilation, so I may miss something in the noise.

fix python3

make type check actually work

vitalybuka marked 2 inline comments as done.

Fix argument instead of failing with error

vitalybuka added inline comments.Apr 22 2021, 12:14 PM
llvm/utils/lit/lit/formats/googletest.py
158

Having that it's trivial to fix the argument we can just do that instead of raising fatal error.

This revision is now accepted and ready to land.Apr 25 2021, 3:39 PM
This revision was automatically updated to reflect the committed changes.