Page MenuHomePhabricator

Restore lit feature object-emission
ClosedPublic

Authored by nigelp-xmos on Mar 12 2021, 7:15 AM.

Details

Summary

D73568 removed the lit feature object-emission, because it was introduced for a
target which did not support the integrated assembler, and that target no longer
required the feature. XCore still does not support the integrated assembler,
so a build with XCore as the default target fails tests requiring
object-emission. This issue was not publicly visible because there was not a
buildbot for XCore as the default target. We fixed the failures downstream. We
now have builder clang-xcore-ubuntu-20-x64 on the staging buildmaster, which
shows the failures. We would like to fix them upstream.

Diff Detail

Event Timeline

nigelp-xmos created this revision.Mar 12 2021, 7:15 AM
nigelp-xmos requested review of this revision.Mar 12 2021, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 7:15 AM

Invite reviewers around lit.cfg.py changes.

For test/DebugInfo/Generic/, can you exclude xcore in lit.cfg.py instead of patching nearly every file?

@MaskRay Thanks for the comment. I will look at that. But DebugInfo/Generic has 78 files requiring object-emission and 60 not requiring it - isn't it worth still running the 60? (I looked at some to see what they were doing: running llvm-as, llvm-dis, and checking assembly output.)

nigelp-xmos added a comment.EditedWed, Mar 31, 9:14 AM

DebugInfo/Generic can be handled by two lines in lit.local.cfg:

elif 'xcore' in config.target_triple:
config.unsupported = True

If that is better than qualifying the individual tests, I'll rebase and upload the patch.

MaskRay added a comment.EditedWed, Mar 31, 10:53 AM

There are other targets which don't support/use integrated assembler but they are not host targets, so that is fine.
Now you want to run XCore as something similar like a host target and exercise these tests, I think it is reasonable to add more requirement like supporting integrated assembler.
(IIRC we requested the same for M68k.)
Otherwise new tests may be developed with subsequent fixups that add REQUIRES: object-emission. That is certainly a maintenance burden.

This is a phase ordering issue. If an integrated assembler is planned, adding REQUIRES: object-emission and then removing REQUIRES: object-emission is just churn.
If there are few tests in some directory which needs REQUIRES: object-emission, I think adding the annotation is fine if that conveniences you.

Thanks, I see what you mean about churn and maintenance. The integrated assembler is planned. I will rework the patch.

Do you mean that a build for a host would normally have that host as its default target, and the generic tests only need to pass for that target?

nigelp-xmos updated this revision to Diff 334663.EditedThu, Apr 1, 5:53 AM

Omit DebugInfo/Generic on XCore to avoid annotating 70 separate files.

As suggested, excluded xcore from DebugInfo/Generic rather than patching each file.

We previously found a code issue by running default-target tests as xcore (D92108), so it was worth doing. Is it not right to run like this? @MaskRay I must admit I don't understand how this is running like a host target.

Invite debug info code owner to review.

JDevlieghere accepted this revision.Thu, Apr 15, 2:02 PM

LGTM if @MaskRay is happy.

This revision is now accepted and ready to land.Thu, Apr 15, 2:02 PM
MaskRay accepted this revision.Thu, Apr 15, 3:59 PM

Thank you for the reviews, I appreciate people taking time on something that is only for this target.