Page MenuHomeFreeBSD

libclang_rt: enable on powerpc*
ClosedPublic

Authored by pkubaj on Sun, Nov 17, 2:52 AM.

Details

Summary

Enable on powerpc64 and in lib/libclang_rt/Makefile change MACHINE_CPUARCH to MACHINE_ARCH because on powerpc64 MACHINE_ARCH==MACHINE_CPUARCH so the 32-bit library overwrites 64-bit library during installworld.

This patch doesn't enable any other libclang_rt libraries because they need to be separately ported.

I have verified that games/julius (which fails on powerpc64 elfv2 without this change because of no libclang_rt profiling library) builds.

Test Plan

Ship it, test on powerpc and powerpcspe

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

pkubaj created this revision.Sun, Nov 17, 2:52 AM
pkubaj updated this revision to Diff 64479.Sun, Nov 17, 2:55 AM

Also add new libraries to tools/build/mk/OptionalObsoleteFiles.inc.

imp added inline comments.Sun, Nov 17, 2:58 AM
lib/Makefile
163 ↗(On Diff #64479)

This enables it for 32 bit powerpc. Is that intended?

pkubaj added inline comments.Sun, Nov 17, 3:46 PM
lib/Makefile
163 ↗(On Diff #64479)

Yes, the title is wrong, the point is to enable it on all powerpc architectures.

pkubaj retitled this revision from libclang_rt: enable on powerpc64 to libclang_rt: enable on powerpc*.Sun, Nov 17, 3:46 PM
jhibbits added inline comments.Tue, Nov 19, 2:35 AM
lib/libclang_rt/Makefile.inc
11 ↗(On Diff #64479)

Is this supposed to be a different change? powerpc isn't amd64.

pkubaj added inline comments.Tue, Nov 19, 10:07 AM
lib/libclang_rt/Makefile.inc
11 ↗(On Diff #64479)

No, it's supposed to be here. The reason is that MACHINE_CPUARCH on both powerpc and powerpc64 are the same (powerpc). This means that 64-bit and 32-bit library will have the same name and during installworld 32-bit library will overwrite the 64-bit one. Changing to MACHINE_ARCH will mean libraries will have different names.

jhibbits accepted this revision.Tue, Nov 19, 1:28 PM
jhibbits added inline comments.
lib/libclang_rt/Makefile.inc
11 ↗(On Diff #64479)

Oh right, I don't know what I was thinking when I read that.

This revision is now accepted and ready to land.Tue, Nov 19, 1:28 PM
dim accepted this revision.Wed, Nov 20, 7:04 PM

LGTM.

linimon resigned from this revision.Wed, Nov 20, 10:35 PM

This is simply above my pay-grade.

pkubaj added a subscriber: linimon.Thu, Nov 21, 9:33 AM

This is simply above my pay-grade.

I need a mentor approval to commit it, so either you or mat or tcberner approve of it as well, or some src committer will have to commit on their own :) games/julius might seem not very useful, but lack of patch also breaks math/suitesparse, which makes 422 other ports skipped, so I imagine this is pretty important.

either you or mat or tcberner approve of it as well, or some src committer will have to commit on their own

You need a src committer to approve in any case. IIUC mentorship doesn't cross repository boundaries.

A quick check shows that none of the 3 of us have src bits.

My understanding is you need both mentor approval and src approval. Consider it src approved by me.

dim added a comment.Thu, Nov 21, 6:50 PM

@pkubaj if you can't commit it, I will be happy to do so.

In D22425#491696, @dim wrote:

@pkubaj if you can't commit it, I will be happy to do so.

I technically can, but my mentors will be angry :)

If you can commit it, please do.

This revision was automatically updated to reflect the committed changes.