Page MenuHomePhabricator

[Builtins] Provide a mechanism to selectively disable tests based on whether an implementation is provided by a builtin library.
ClosedPublic

Authored by delcypher on Sep 25 2019, 5:33 PM.

Details

Summary

If a platform removes some builtin implementations (e.g. via the
Darwin-excludes mechanism) then this can lead to test failures because
the test expects an implementation to be available.

To solve this lit features are added for each configuration based
on which sources are included in the builtin library. The features
are of the form librt_has_<name> where <name> is the name of the
source file with the file extension removed. This handles C and
assembly sources.

With the lit features in place it is possible to make certain tests
require them.

Example:

REQUIRES: librt_has_comparedf2

All top-level tests in test/builtins/Unit (i.e. not under
arm, ppc, and riscv) have been annotated with the appropriate
REQUIRES: librt_has_* statement.

rdar://problem/55520987

Diff Detail

Event Timeline

delcypher created this revision.Sep 25 2019, 5:33 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 25 2019, 5:33 PM
Herald added subscribers: Restricted Project, mgorny. · View Herald Transcript
delcypher updated this revision to Diff 222668.Oct 1 2019, 11:54 AM

Add sanity checks to lit config.

phosek accepted this revision.Oct 1 2019, 11:40 PM

LGTM % a few nits

test/builtins/Unit/lit.cfg.py
101 ↗(On Diff #222668)

No need to check the length, if builtins_source_features: is sufficient.

108 ↗(On Diff #222668)

I'd slightly prefer to do:

if builtin_source_feature not in builtins_source_features_set:
  builtins_source_features_set.add(builtin_source_feature)
else:
  builtins_source_feature_duplicates.append(builtin_source_feature)

It seems easier to comprehend.

This revision is now accepted and ready to land.Oct 1 2019, 11:40 PM
delcypher marked an inline comment as done.Oct 2 2019, 3:03 PM
delcypher added inline comments.
test/builtins/Unit/lit.cfg.py
108 ↗(On Diff #222668)

Agreed. That is easier to read. The original code I wrote was written that way is to avoid doing two set look ups, instead the original code only does one.

However this is probably a very premature optimization at the expense of readability so let's go with the more readable option for now.

delcypher marked an inline comment as not done.Oct 2 2019, 3:32 PM
delcypher updated this revision to Diff 223950.Oct 8 2019, 3:04 PM
  • Rename crt_has_* -> librt_has_* so consistency with %librt lit substitution.
  • Annotate as many tests as possible.
  • Make python fixups suggested by Petr Hosek.

@beanz @steven_wu @arphaman @phosek @dexonsmith @thakis I've improved this patch by annotating as many tests as possible. Okay to land?

delcypher edited the summary of this revision. (Show Details)Oct 17 2019, 10:58 AM
This revision was automatically updated to reflect the committed changes.

Damn looks like something is broken for linux powerpc

http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/21208/steps/ninja%20check%201/logs/stdio

llvm-lit: /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/projects/compiler-rt/test/builtins/Unit/lit.cfg.py:116: fatal: builtins_source_features contains duplicates: ['librt_has_divtc3']

I'm going to turn this into a warning to unbreak the bot for now.

@joerg @rupprecht Any idea what's going on here? My CMake code jsut reads the source files used to built the builtin library. My guess is that both lib/builtins/ppc/divtc3.c and lib/builtins/divtc3.c are used to build the builtin library for powerpc. That sounds like a bug. I'd expect that to cause a linker error with multiple symbols of the same name being defined.

Landed workaround to downgrade fatal error to warning in r375162.

@joerg @rupprecht

So lib/builtins/CMakeLists.txt has

set(GENERIC_SOURCES
  absvdi2.c
  ....
  divtc3.c
  ...
)

and then later

set(powerpc64_SOURCES
  ppc/divtc3.c
  ppc/fixtfdi.c
  ppc/fixunstfti.c
  ppc/fixunstfdi.c
  ppc/floattitf.c
  ppc/floatditf.c
  ppc/floatunditf.c
  ppc/gcc_qadd.c
  ppc/gcc_qdiv.c
  ppc/gcc_qmul.c
  ppc/gcc_qsub.c
  ppc/multc3.c
  ${GENERIC_SOURCES}
)

so it looks like both lib/builtins/divtc3.c and lib/builtins/ppc/divtc3.c are used for the builtin library for powerpc. I'm guessing rather than using set(powerpc64_SOURCES ... ${GENERIC_SOURCES}) we need a function that produces a list with files with duplicate filenames (not full path) removed.

Damn looks like something is broken for linux powerpc

http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/21208/steps/ninja%20check%201/logs/stdio

llvm-lit: /home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/projects/compiler-rt/test/builtins/Unit/lit.cfg.py:116: fatal: builtins_source_features contains duplicates: ['librt_has_divtc3']

I'm going to turn this into a warning to unbreak the bot for now.

@joerg @rupprecht Any idea what's going on here? My CMake code jsut reads the source files used to built the builtin library. My guess is that both lib/builtins/ppc/divtc3.c and lib/builtins/divtc3.c are used to build the builtin library for powerpc. That sounds like a bug. I'd expect that to cause a linker error with multiple symbols of the same name being defined.

I think you have the right idea. From my reading:

  1. All the builtins are added to GENERIC_SOURCES: e.g. divtc3 here: https://github.com/llvm/llvm-project/blob/2ca8e27bd038673b514c8452d661a413ce0d2123/compiler-rt/lib/builtins/CMakeLists.txt#L68
  2. Per-arch builtins get appended to that list: e.g. for ppc/divtc3 here: https://github.com/llvm/llvm-project/blob/2ca8e27bd038673b514c8452d661a413ce0d2123/compiler-rt/lib/builtins/CMakeLists.txt#L521
  3. Generic versions are later filtered out if there's a arch-specific builtin: https://github.com/llvm/llvm-project/blob/2ca8e27bd038673b514c8452d661a413ce0d2123/compiler-rt/lib/builtins/CMakeLists.txt#L607

If that's where the sources is coming from, why is it not being filtered when this cmake config looks at it? Is it not running because of the if (APPLE) before it?

(Huge caveat: I spend almost no time developing in cmake, so I don't really know what I'm looking at)

Hmm this code looks a little dubious. It's modifying a list while iterating over it and I'm not sure that regex is right.

# Filter out generic versions of routines that are re-implemented in
      # architecture specific manner.  This prevents multiple definitions of the
      # same symbols, making the symbol selection non-deterministic.
      foreach (_file ${${arch}_SOURCES})
        if (${_file} MATCHES ${_arch}/*)
          get_filename_component(_name ${_file} NAME)
          string(REPLACE ".S" ".c" _cname "${_name}")
          list(REMOVE_ITEM ${arch}_SOURCES ${_cname})
        endif ()
      endforeach ()

Okay I figured out the issue. The problem is with the filtering code. It assumes that architecture specific specializations have a filepath like <arch>/filename. The problem here is that for powerpc64 the <arch> is powerpc64 but the directory name used is actually ppc. I've got a fix for this which should prevent people from making the same mistake in the future.

Fix for powerpc issue is now up for review: https://reviews.llvm.org/D69189