compnerd added inline comments.
================
Comment at: CodeGen/CodeGenModule.cpp:2957-2958
!getCodeGenOpts().LTOVisibilityPublicStd &&
- !getTriple().isWindowsGNUEnvironment()) {
+ !getTriple().isWindowsGNUEnvironment() &&
+ !getTriple().isWindowsMSVCEnvironment()) {
const FunctionDecl *FD = GetRuntimeFunctionDecl(Context, Name);
----------------
rnk wrote:
> compnerd wrote:
> > rnk wrote:
> > > The condition here of `T.isOSBinFormatCOFF() &&
> > > !T.isWindowsGNUEnvironment() && !T.isWindowsMSVCEnvironment()` is
> > > essentially equivalent to `T.isWindowsItaniumEnvironment()`. Please
> > > simplify the condition to that. The other relevant environments are
> > > Cygnus for Cygwin and CoreCLR, which probably want to follow the MSVC/GNU
> > > behavior.
> > IIRC, one of the issues was that lldb couldn't deal very well with the
> > thunks. The other thing is that there is a small penalty for something
> > that we know that we are going to redirect through. However, I think that
> > if lldb is able to deal with the thunks now, I would be okay with the
> > penalty to make the behavior more similar to MSVC. Basically, if lldb
> > works, lets just get rid of the special behavior for the itanium
> > environment.
> Can you elaborate on what didn't work in LLDB when the thunks were present?
> What kind of functionality did you have to exercise to get it to misbehave?
Sure, it was single stepping through an indirected function call. It would put
you into the thunk rather than the destination.
================
Comment at: CodeGenObjC/gnu-init.m:103
+// Make sure we do not have dllimport on the load function
+// CHECK-WIN: declare dso_local void @__objc_load
----------------
rnk wrote:
> compnerd wrote:
> > rnk wrote:
> > > mgrang wrote:
> > > > I had to remove dllimport in this and the next unit test. I am not sure
> > > > of the wider implications of this change. Maybe controlling this via a
> > > > flag (like -flto-visibility-public-std) is a better/more localized
> > > > option?
> > > These are the ones that I think @compnerd really cares about since the
> > > objc runtime is typically dynamically linked and frequently called. We
> > > might want to arrange things such that we have a separate codepath for
> > > ObjC runtime helpers. I'm surprised we don't already have such a code
> > > path.
> > I think that @theraven would care more about this code path than I. I
> > think that this change may be wrong, because the load here is supposed to
> > be in the ObjC runtime, and this becoming local to the module would break
> > the global registration.
> We just set dso_local whenever something isn't dllimport, even if it's a lie
> sometimes. I think everything would work as intended with a linker thunk.
> It's "true" as far as LLVM is concerned for the purposes of emitting
> relocations.
Ah, okay, then, this might be okay. However, the use of `dllimport` here would
force that the runtime to be called. Then again it is possible to statically
link ...
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55229/new/
https://reviews.llvm.org/D55229
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
