Page MenuHomePhabricator

clang: Remove __atomic libstdc++ hack
ClosedPublic

Authored by urnathan on Apr 14 2021, 4:31 AM.

Details

Summary

This hack was added back in 2012 for libstdc++4.6. The last 4.6
release was 2013. The commit was 45bb8855e0ecc, Richard Smith, Thu
Oct 4 22:13:39 2012 +0000. If someone really wants to use clang with
such a libstdc++, well, they know where to get one :)

but that;s just MHO

Diff Detail

Event Timeline

urnathan created this revision.Apr 14 2021, 4:31 AM
urnathan requested review of this revision.Apr 14 2021, 4:31 AM

I am fine removing this hack for such an old version of libstdc++, but it looks like there are two test failures that should be addressed:

https://reviews.llvm.org/harbormaster/unit/view/552317/
https://reviews.llvm.org/harbormaster/unit/view/552314/

One question I do have is whether we have a documented compatibility statement anywhere about what versions of libstdc++ (or libc++ for that matter) we support and whether that needs to be updated. I couldn't find one when I looked, but I may have looked in the wrong places.

Also, should we mention something in the release notes about no longer supporting this version of libstdc++?

jfb added a reviewer: jwakely.Apr 16 2021, 9:25 AM

Not a libstdc++ or Clang maintainer, but I think it's reasonable to remove this.

jwakely accepted this revision.Apr 16 2021, 10:23 AM

LGTM.

I don't even remember those namespaces existing, but I don't think it's reasonable to expect current clang to support ancient (and broken) headers.

This revision is now accepted and ready to land.Apr 16 2021, 10:23 AM

One question I do have is whether we have a documented compatibility statement anywhere about what versions of libstdc++ (or libc++ for that matter) we support and whether that needs to be updated. I couldn't find one when I looked, but I may have looked in the wrong places.

Aha, I *knew* we had something somewhere. We should probably update this: https://github.com/llvm/llvm-project/blob/main/clang/docs/Toolchain.rst#libstdc-gnu

Not sure if I should have "accepted" this, I'm not an approver.

aaron.ballman requested changes to this revision.Apr 16 2021, 10:39 AM

Not sure if I should have "accepted" this, I'm not an approver.

No worries! FWIW, the patch approval documentation is at https://llvm.org/docs/CodeReview.html#lgtm-how-a-patch-is-accepted but basically, anyone can approve but code should only go in once there's consensus including people with technical expertise in the area of the code being changed.

I'll "request changes" so it's clear there's a bit more to be done here.

This revision now requires changes to proceed.Apr 16 2021, 10:39 AM
urnathan updated this revision to Diff 338189.Apr 16 2021, 11:42 AM

Fix failing test -- I must have confused myself

urnathan updated this revision to Diff 338193.Apr 16 2021, 11:54 AM

Added ReleaseNote about libstdc++ -- is that the best place to document such a requirement?

From a process standpoint, I think the best thing would be to first send out a patch adding a release note saying we no longer support libstdc++ < 4.8.3 and updating the part of docs/Toolchain.rst that Aaron linked to, and send a brief note to cfe-dev drawing people's attention to that patch and asking for objections -- I don't think you need to wait for more than a couple of days for such objections, but as this is effectively a policy change we should probably give people a little time to respond. Then remove the workarounds as (hopefully now entirely uncontroversial) follow-up steps.

clang/test/SemaCXX/libstdcxx_atomic_ns_hack.cpp
6

Then remove the workarounds as (hopefully now entirely uncontroversial) follow-up steps.

Actually, you know what, I don't really see the point in splitting up these two steps. So I think my suggestion reduces to: let's mail cfe-dev to draw attention to this and go ahead in a couple of days if we don't have sustained objections.

Then remove the workarounds as (hopefully now entirely uncontroversial) follow-up steps.

Actually, you know what, I don't really see the point in splitting up these two steps. So I think my suggestion reduces to: let's mail cfe-dev to draw attention to this and go ahead in a couple of days if we don't have sustained objections.

+1, I think that should alleviate most concerns.

urnathan updated this revision to Diff 338209.Apr 16 2021, 12:54 PM

Ok, hopefully right this time. I went with v4.9, as that's the version Richard mentioned in D100469, I just confused myself with 4.8.3. will email cfe-dev

urnathan updated this revision to Diff 338504.Apr 19 2021, 6:27 AM

Change to 4.8.3 as oldest version

urnathan updated this revision to Diff 338940.Apr 20 2021, 11:31 AM

Set 4.8.3 as oldest supported

aaron.ballman accepted this revision.Apr 21 2021, 7:03 AM

LGTM assuming community doesn't find a reason to change the supported version for some reason.

This revision is now accepted and ready to land.Apr 21 2021, 7:03 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2021, 5:26 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript