vsapsai added a comment.
In https://reviews.llvm.org/D45015#1188141, @EricWF wrote:
> Hi, I'm in the process of moving cities. please take over.
>
> Thanks, and sorry
No worries. Thanks for spending time on this issue and making the fix,
committing is the easy part. Good luck with the move.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL338934: [Preprocessor] Allow libc++ to detect when aligned
allocation is unavailable. (authored by vsapsai, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
EricWF added a comment.
Hi, I'm in the process of moving cities. please take over.
Thanks, and sorry
https://reviews.llvm.org/D45015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
vsapsai added a comment.
Eric, will you have time to commit this patch? If you don't have time and don't
have objections, I plan to land this change on your behalf.
https://reviews.llvm.org/D45015
___
cfe-commits mailing list
rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.
Comment at: lib/Driver/ToolChains/Darwin.cpp:2027
+ isAlignedAllocationUnavailable())
CC1Args.push_back("-faligned-alloc-unavailable");
}
Quuxplusone added inline comments.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6425
+ "available on %2 %3 or newer">;
def note_silence_unligned_allocation_unavailable : Note<
"if you supply your own aligned allocation functions, use "
I observe
EricWF added a reviewer: dexonsmith.
EricWF added a comment.
Ping.
Are there any more reviewers I should add to this?
https://reviews.llvm.org/D45015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
EricWF added a comment.
@rsmith Ping. This needs to land before the next branch for release.
https://reviews.llvm.org/D45015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
vsapsai added a comment.
With this change and the mentioned libc++ change the tests with old libc++
dylib are passing (didn't test all possible configurations though). Would like
to get more feedback from other reviewers on this matter.
https://reviews.llvm.org/D45015
EricWF updated this revision to Diff 150635.
EricWF added a comment.
Remove `-nostdinc++` check as requested.
https://reviews.llvm.org/D45015
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
lib/Driver/ToolChains/Darwin.cpp
EricWF added a comment.
In https://reviews.llvm.org/D45015#1127046, @vsapsai wrote:
> In https://reviews.llvm.org/D45015#1121762, @EricWF wrote:
>
> > In https://reviews.llvm.org/D45015#1121581, @ahatanak wrote:
> >
> > > Could you elaborate on what kind of changes you are planning to make in
>
vsapsai added a comment.
In https://reviews.llvm.org/D45015#1121762, @EricWF wrote:
> In https://reviews.llvm.org/D45015#1121581, @ahatanak wrote:
>
> > Could you elaborate on what kind of changes you are planning to make in
> > libc++ after committing this patch?
>
>
> Libc++ shouldn't
vsapsai added a comment.
Sorry for the churn but can you please take out `-nostdinc++` part out of this
change? After more thinking and discussion we think there is a chance
developers can use `-nostdinc++` not only for building the standard library.
`-nostdinc++` is a signal of building the
EricWF added a comment.
In https://reviews.llvm.org/D45015#1123164, @ahatanak wrote:
> In https://reviews.llvm.org/D45015#1123097, @EricWF wrote:
>
> > In https://reviews.llvm.org/D45015#1121874, @ahatanak wrote:
> >
> > > I see, thank you.
> > >
> > > clang front-end currently fails to issue a
ahatanak added a comment.
In https://reviews.llvm.org/D45015#1123097, @EricWF wrote:
> In https://reviews.llvm.org/D45015#1121874, @ahatanak wrote:
>
> > I see, thank you.
> >
> > clang front-end currently fails to issue a warning or error when an aligned
> > allocation/deallocation functions
EricWF added a comment.
In https://reviews.llvm.org/D45015#1121874, @ahatanak wrote:
> I see, thank you.
>
> clang front-end currently fails to issue a warning or error when an aligned
> allocation/deallocation functions are required but not available in a few
> cases (e.g., delete called from
ahatanak added a comment.
I see, thank you.
clang front-end currently fails to issue a warning or error when an aligned
allocation/deallocation functions are required but not available in a few cases
(e.g., delete called from a deleting destructor, calls to operator or builtin
operator
EricWF added a comment.
In https://reviews.llvm.org/D45015#1121581, @ahatanak wrote:
> Could you elaborate on what kind of changes you are planning to make in
> libc++ after committing this patch?
Libc++ shouldn't actually need any changes if this current patch lands.
Currently libc++ is in
ahatanak added a comment.
Could you elaborate on what kind of changes you are planning to make in libc++
after committing this patch?
https://reviews.llvm.org/D45015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
EricWF updated this revision to Diff 149596.
EricWF edited the summary of this revision.
EricWF added a comment.
Update the patch with the form suggested in the previous conversation. See the
updated summary for a description of the behavior.
The Darwin driver no longer passes
EricWF added a comment.
In https://reviews.llvm.org/D45015#1119286, @ahatanak wrote:
> In https://reviews.llvm.org/D45015#1105388, @rsmith wrote:
>
> > In https://reviews.llvm.org/D45015#1105372, @vsapsai wrote:
> >
> > > What when compiler has `__builtin_operator_new`,
> > >
EricWF added a comment.
The new path forward sounds good to me. I'll work on implementing it.
Repository:
rC Clang
https://reviews.llvm.org/D45015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
ahatanak added a comment.
I think clang should error out or warn when aligned operator or builtin
operator new/delete functions are used when they are not available (r306722
should have handled them).
I'm also not sure not defining `__cpp_aligned_new` is sufficient. My
understanding is that
ahatanak added a comment.
In https://reviews.llvm.org/D45015#1105388, @rsmith wrote:
> In https://reviews.llvm.org/D45015#1105372, @vsapsai wrote:
>
> > What when compiler has `__builtin_operator_new`,
> > `__builtin_operator_delete`? If I build libc++ tests with recent Clang
> > which has
rsmith added a comment.
In https://reviews.llvm.org/D45015#1109049, @ahatanak wrote:
> - Currently clang errors out when aligned operator new is selected but the
> OS's version is too old to support it. What's the reason we want to change
> this now to be a warning rather than an error?
I
vsapsai added a comment.
Somewhat tangential, in discussion with Duncan he mentioned that `-nostdinc++`
should turn off assumptions about old Darwin. So if you build libc++ yourself,
you don't care what does the system stdlib support. I agree with that and think
it doesn't interfere with the
ahatanak added a comment.
In https://reviews.llvm.org/D45015#1105314, @rsmith wrote:
> Hmm, perhaps our strategy for handling aligned allocation on Darwin should be
> revisited. We shouldn't be defining `__cpp_aligned_allocation` if we believe
> it doesn't work -- that will break code that
rsmith added a comment.
In https://reviews.llvm.org/D45015#1105372, @vsapsai wrote:
> What when compiler has `__builtin_operator_new`, `__builtin_operator_delete`?
> If I build libc++ tests with recent Clang which has these builtins and run
> tests with libc++.dylib from old Darwin, there are
vsapsai added a comment.
In https://reviews.llvm.org/D45015#1105314, @rsmith wrote:
> That is: on old Darwin, we should not define `__cpp_aligned_allocation` (even
> in C++17), produce the "no aligned allocation support" warning in C++17 mode,
> and then not try to call the aligned allocation
rsmith added a comment.
Hmm, perhaps our strategy for handling aligned allocation on Darwin should be
revisited. We shouldn't be defining `__cpp_aligned_allocation` if we believe it
doesn't work -- that will break code that uses aligned allocation where
available and falls back to something
vsapsai added a comment.
Eric, do you have more thoughts on this issue?
Repository:
rC Clang
https://reviews.llvm.org/D45015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
vsapsai added a comment.
In https://reviews.llvm.org/D45015#1064930, @EricWF wrote:
> In https://reviews.llvm.org/D45015#1064922, @vsapsai wrote:
>
> > Another approach is `__has_feature` but I don't think it is applicable in
> > this case.
> >
> > Is there a way right now to detect that
EricWF added a comment.
In https://reviews.llvm.org/D45015#1064922, @vsapsai wrote:
> Another approach is `__has_feature` but I don't think it is applicable in
> this case.
>
> Is there a way right now to detect that aligned allocation is supported by
> clang, regardless of link time? Asking
vsapsai added a comment.
Another approach is `__has_feature` but I don't think it is applicable in this
case.
Is there a way right now to detect that aligned allocation is supported by
clang, regardless of link time? Asking to make sure we are consistent.
Repository:
rC Clang
EricWF added a comment.
Ping. I would like to get a way for libc++ to detect this case before the next
release.
Repository:
rC Clang
https://reviews.llvm.org/D45015
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
EricWF created this revision.
EricWF added reviewers: rsmith, vsapsai, erik.pilkington, ahatanak.
Libc++ needs to know when aligned allocation is supported by clang, but is
otherwise unavailable at link time. This patch adds a predefined macro to allow
libc++ to do that.
IDK if using a
36 matches
Mail list logo
