Page MenuHomePhabricator

[libcxx] Add -fno-exceptions libcxx builders to zorg
ClosedPublic

Authored by rmaprath on Nov 3 2015, 10:19 AM.

Details

Summary

This patch adds the necessary builders to zorg to get the -fno-exceptions libcxx library variant building. I've only used those build-slaves which I have got explicit permission to use for this purpose.

As discussed on the cfe-dev list, these builders won't run the libcxx test suite (they will, in the near future, when we adapt the tests).

These builders will ensure that we don't break the -fno-exceptions library build. This patch can only go in once D14292 is sorted.

Diff Detail

Event Timeline

rmaprath updated this revision to Diff 39083.Nov 3 2015, 10:19 AM
rmaprath retitled this revision from to [libcxx] Add -fno-exceptions libcxx builders to zorg.
rmaprath updated this object.
rmaprath added a subscriber: cfe-commits.

PS: I've run a couple of hand-crafted builders on a locally hacked up buildbot and made sure the above changes are fine.

gribozavr edited edge metadata.Nov 3 2015, 10:34 AM

The Debian configuration LGTM.

rengolin added inline comments.Nov 3 2015, 12:06 PM
buildbot/osuosl/master/config/builders.py
868

I'd like to run this internally first, as I'm not sure it will work. I'll use this configuration and will let you know if it's ok.

We should only put this up if it's totally green.

jroelofs added inline comments.
buildbot/osuosl/master/config/builders.py
768

How bad is the fallout?

rmaprath added inline comments.Nov 3 2015, 1:22 PM
buildbot/osuosl/master/config/builders.py
768

I see about 160 test failures. Mostly because those tests have try/catch/throw statements that don't compile under -fno-exceptions, but there are other kinds of failures as well.

One option is to add xfails for those tests and gradually fix them up, this will ensure that we don't regress those tests that are already passing. I've taken the slightly more involved approach and started fixing the tests all in one go. I plan to send a follow-up patch soon with the general idea of the proposed "fix".

rmaprath added inline comments.Nov 3 2015, 1:35 PM
buildbot/osuosl/master/config/builders.py
868

Thanks! The newly added Debian configuration is quite similar to the configuration I ran locally. I have run some ARM configurations but it is best to run this particular configuration if possible.

You'd need the patch for D14292 though. I was hoping to get it committed first, so that these configs can be verified easily.

rengolin edited edge metadata.Nov 4 2015, 3:41 AM

Hi Asiri,

Can I propose a different approach?

We now have a silent buildbot, which will never email people about breakages, but can be publicly monitored by you, me and others. I'm assuming you have access to at least one x86 and one ARM machines, so that you could set up a local buildbot, reporting to the buildmaster, without upsetting anyone else, but where I, Jon, Marshall can also see the progress of all tests being fixed, thus helping approving the patches for future fixes.

You don't need to share any information on the board, just connect to the master and start building...

Once the bot is green, we can add the builds on our own public hardware, avoiding the need for disabling the tests.

cheers,
--renato

Hi Asiri,

Can I propose a different approach?

We now have a silent buildbot, which will never email people about breakages, but can be publicly monitored by you, me and others. I'm assuming you have access to at least one x86 and one ARM machines, so that you could set up a local buildbot, reporting to the buildmaster, without upsetting anyone else, but where I, Jon, Marshall can also see the progress of all tests being fixed, thus helping approving the patches for future fixes.

You don't need to share any information on the board, just connect to the master and start building...

Once the bot is green, we can add the builds on our own public hardware, avoiding the need for disabling the tests.

cheers,
--renato

Counter proposal, we mark all the currently failing (-fno-exceptions) test cases with:

// XFAIL: libcpp-no-exceptions

This has the following advantages:

  • No need to change buildbots to skip tests
  • The bots will catch any regressions in those tests that are already passing under -fno-exceptions
  • You will be able to monitor my progress with the test fixes using the public (-fno-exceptions) buildbots
  • I don't have to setup local buildbots (this is very difficult for us, infrastructure / legal problems)

What do you think?

  • Asiri

Counter proposal, we mark all the currently failing (-fno-exceptions) test cases with:

// XFAIL: libcpp-no-exceptions

This has the following advantages:

  • No need to change buildbots to skip tests
  • The bots will catch any regressions in those tests that are already passing under -fno-exceptions
  • You will be able to monitor my progress with the test fixes using the public (-fno-exceptions) buildbots
  • I don't have to setup local buildbots (this is very difficult for us, infrastructure / legal problems)

What do you think?

I think this ^ is a reasonable solution.

Jon

  • Asiri

I think this ^ is a reasonable solution.

I'm fine with that, as long as everyone's happy.

160 XFAILs are ok (as long as you're fixing them), disabling the tests makes no sense. :)

cheers,
--renato

I think this ^ is a reasonable solution.

I'm fine with that, as long as everyone's happy.

160 XFAILs are ok (as long as you're fixing them), disabling the tests makes no sense. :)

cheers,
--renato

Great! I will update the patch D14292 to include the X-FAILS and then update the current patch to remove the hack which prevents the tests being run.

Thanks.

  • Asiri
rmaprath updated this revision to Diff 39806.Nov 10 2015, 5:22 AM
rmaprath edited edge metadata.

Updated patch to remove the skipping of tests. Now that we have the appropriate XFAILs in place, these two builders should be green.

I've locally tested the x86 run, I expect the ARM run to be fairly similar.

@Renato: could you kindly test this builder locally? Just in case.

Thanks.

  • Asiri
jroelofs added inline comments.Nov 10 2015, 6:54 AM
buildbot/osuosl/master/config/builders.py
880

The order here seems a bit off to me. I think it should be:

lit_extra_opts={'link_flags': '"-lc++abi -lpthread -lunwind -ldl -lc -lm  -L/opt/llvm/lib/clang/3.6.0/lib/linux -lclang_rt.builtins-arm"'},
rmaprath added inline comments.Nov 10 2015, 7:10 AM
buildbot/osuosl/master/config/builders.py
880

@rengolin: If you are OK with Jon's suggestion, I can update both the configurations (I copied these lines from your existing A15 builder).

Would it be OK if I commit the x86 buildbot changes (Dmitri approved these earlier) while the ARM buildbot changes are being reviewed? I suppose the changes would only take effect once @gkistanova restarts/reconfigs the build-master ?

Cheers,

  • Asiri

Would it be OK if I commit the x86 buildbot changes (Dmitri approved these earlier) while the ARM buildbot changes are being reviewed? I suppose the changes would only take effect once @gkistanova restarts/reconfigs the build-master ?

Cheers,

  • Asiri

That sounds reasonable to me. Be sure to notify Galina once you've made the change.

@rengolin: Gentle ping.

Cheers,

  • Asiri
rengolin accepted this revision.Feb 11 2016, 12:58 PM
rengolin edited edge metadata.

Sorry, this fell out of my radar. I'm ok with the changes. Ping me when you commit so I can monitor the bot for a while.

This revision is now accepted and ready to land.Feb 11 2016, 12:58 PM

Sorry, this fell out of my radar. I'm ok with the changes. Ping me when you commit so I can monitor the bot for a while.

Sorry, I missed your message this time around :)

I'll double-check the patch and commit. Thanks!

/ Asiri

rengolin closed this revision.Jun 27 2016, 6:45 AM