rjmccall added inline comments.
================ Comment at: lib/CodeGen/CGObjCGNU.cpp:3542 + allSelectors.push_back(entry.first); + std::sort(allSelectors.begin(), allSelectors.end()); ---------------- 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? ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:915 + return name; + } /// The GCC ABI superclass message lookup function. Takes a pointer to a ---------------- 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. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:2262 llvm::Constant *CGObjCGNUstep::GetEHType(QualType T) { + if (CGM.getContext().getTargetInfo().getTriple().isWindowsMSVCEnvironment()) + return CGM.getCXXABI().getAddrOfRTTIDescriptor(T); ---------------- theraven wrote: > rjmccall wrote: > > I think this is the third different way that you test for Windows in this > > patch. :) Is there a reason why this is just testing for MSVC and the > > others are testing for PE/COFF or for Windows generally? > For the EH logic, we are using DWARF exceptions if we are targeting a Windows > environment that uses DWARF EH and SEH-based exceptions if we are targeting a > Windows environment that uses MSVC-compatible SEH. > > The section code is specific to PE/COFF, where we don't get to use some of > the ELF tricks. > > The blocks code is specific to Windows, because it relates to Windows > run-time linking. Hypothetically, a PE/COFF platform could provide dynamic > relocations that would eliminate the need for that check. > > It's possible that some of the PE/COFF vs Windows distinctions are wrong. > For example, UEFI images are PE/COFF binaries and if anyone wanted to use > blocks in a UEFI firmware image then they may find that they need the Windows > code path (but I do not have a good way of testing this, so restricted it to > Windows initially). Similarly, some of the section initialisation code in > CGObjCGNU may actually be Windows-only, but it's probably a good starting > point for anyone who actually wants to use Objective-C in UEFI firmware > (though the final destination for such people is likely to involve padded > cells). Okay, the exception logic makes sense. Please make this a common predicate instead of repeating it in different places. My understanding is that the restriction on the use of `dllimport`-ed symbols is a general PE restriction: the format itself only supports binds in the import address table, and everything else has just to be a rebase. I mean, of course you can imagine a loader that extends PE to support arbitrary binds, but you can also imagine a loader that extends PE to support just embedding an ELF image. Firmware probably never uses imported symbols. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:3817 + if (isRethrow && CGF.CGM.getTarget().getTriple().isWindowsMSVCEnvironment()) { + CGF.EmitRuntimeCallOrInvoke(ExceptionReThrowFn).setDoesNotReturn(); + } ---------------- 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. ================ Comment at: lib/CodeGen/CGObjCRuntime.cpp:125 llvm::Constant *TypeInfo; + unsigned Flags; }; ---------------- theraven wrote: > rjmccall wrote: > > Please add some sort of comment about the meaning and source of these flags. > These are not well documented anywhere, including in the Clang Microsoft C++ > ABI code that I read to see why exceptions weren't working, but I've added a > small comment that explains why they exist. I have no idea where the values > come from though. Just mentioning that they're the same catch flags used in the Microsoft C++ ABI is all I'm asking for. Although maybe there could be an abstraction for that instead of passing them around separately? 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
