theraven marked an inline comment as done.
theraven added a comment.
I believe that this is now ready to land.
================
Comment at: lib/CodeGen/CGObjCGNU.cpp:915
+ return name;
+ }
/// The GCC ABI superclass message lookup function. Takes a pointer to a
----------------
rjmccall wrote:
> theraven wrote:
> > rjmccall wrote:
> > > Can this section-names cleanup also just be in a separate patch?
> > This is difficult to extract, because it's mostly needed for the COFF part
> > where we need to modify the section names. For ELF, it was fine to keep
> > them as separate `const char*`s
> I don't mind if the extracted patch seems unmotivated on its own, but I do
> think it should be a separate patch.
None of the changes to do this are in a separate commit, and they touch enough
of the code that it's very difficult to separate them without ending up with
conflicts in this branch (which touches the code surrounding the changes in
multiple places). I cannot extract the code in such a way that this patch
would cleanly rebase on top, because various things (e.g. the null section
code) needed restructuring to work with Windows and touch this logic.
================
Comment at: lib/CodeGen/CGObjCGNU.cpp:3817
+ if (isRethrow && CGF.CGM.getTarget().getTriple().isWindowsMSVCEnvironment())
{
+ CGF.EmitRuntimeCallOrInvoke(ExceptionReThrowFn).setDoesNotReturn();
+ }
----------------
rjmccall wrote:
> theraven wrote:
> > rjmccall wrote:
> > > You're sure here that the static information aligns with the dynamic?
> > I'm not sure I understand this question.
> Your new code path no longer uses `ExceptionAsObject`, which is our static
> notion of the current exception value. Instead, you call a runtime function
> which presumably relies on a dynamically-stored current exception value. I'm
> just asking if, in this lexical position, you're definitely rethrowing the
> right exception.
Ah, I see. This code path is hit only as a `@throw;`, not a `@throw obj` (in
the latter case, there is a non-null return from `S.getThrowExpr()`). In this
case, the value of ExceptionAsObject may not be useful. In the case of a
catchall, this is a load from a stack location with no corresponding store.
The Windows unwind logic keeps the object on the stack during funclet execution
and then continues the unwind at the end, without ever providing this frame's
funclets with a pointer to it. That's probably not very obvious, so I've added
an explanatory comment.
================
Comment at: lib/CodeGen/CGObjCGNU.cpp:3542
+ allSelectors.push_back(entry.first);
+ std::sort(allSelectors.begin(), allSelectors.end());
----------------
rjmccall wrote:
> mgrang wrote:
> > Please use llvm::sort instead of std::sort. See
> > https://llvm.org/docs/CodingStandards.html#beware-of-non-deterministic-sorting-order-of-equal-elements.
> Also, why is the sort necessary? Should this change be separately committed
> and tested?
Ooops, this was a fix for PR35277, which was meant to be a separate review. I
forgot to remove it from this branch after cherry-picking it to another one.
Removed.
Repository:
rC Clang
https://reviews.llvm.org/D50144
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
