Page MenuHomePhabricator

Pass -rtlib=libgcc in tests conditioned on the default.

Authored by stellaraccident on Jul 17 2020, 11:15 AM.


  • This test was failing in our builds that configure compiler-rt as the configure-time rtlib.
  • Opted for this test fix instead of a rollback, and hopefully TI can fix forward if this weakens the tests beyond expectations.
  • Suspected this failure introduced in D81676.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2020, 11:15 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
echristo added a subscriber: echristo.

(Adding Sterling as well)
I think this will be an OK workaround for now, go ahead and reply on the main thread as I think they'll want to set the default runtime library as part of the toolchain. I'm surprised at this behavior as well.

echristo accepted this revision.Jul 17 2020, 11:25 AM
This revision is now accepted and ready to land.Jul 17 2020, 11:25 AM
This revision was automatically updated to reflect the committed changes.

This is a reasonable workaround, as it seems to be checking the position of various arguments, and as things work today, lgcc and the compiler-rt variants appear in exactly the same places.

If we wanted to be really strict, we would add some new test that tested that the two different libraries appeared in exactly the same places, but I believe if this became not true we would see many other failures.

atrosinenko added a reviewer: atrosinenko.EditedJul 17 2020, 11:40 AM
atrosinenko added a subscriber: atrosinenko.

If I get it right, the only thing this patch weakens about msp430-toolchain.c test is an assumption that libgcc is used by default.

On one hand, now there is no clang_rt.builtins for MSP430, so testing this assumption makes some sense. Still, it was not intentional at the time of writing. :)

On the other hand, I'm working on porting builtins to MSP430. Even when builtins library is not available and the default rtlib is compiler-rt, this should not introduce some "hidden" errors - just an explicitly failing linker.

So this patch looks quite reasonable to me.