This is needed for AddressSanitizer support on Windows, where we need to pass in the ASAN runtime library names from the external Mozilla build system.

ping?

Comment on attachment 8505176 [details] [diff] [review] Patch (v1) Brian, David, I've been waiting for review on this patch for three months. Can one of you please help review it? It should not take more than 1 minute, and I'm running out of patience here. :-) Thanks!

Comment on attachment 8505176 [details] [diff] [review] Patch (v1) Brian and David aren't NSS peers.

Comment on attachment 8505176 [details] [diff] [review] Patch (v1) Review of attachment 8505176 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. I have some questions/suggested changes. ::: security/nss/coreconf/command.mk @@ +11,5 @@ > AS = $(CC) > ASFLAGS += $(CFLAGS) > CCF = $(CC) $(CFLAGS) > +LINK_DLL = $(LINK) $(OS_DLLFLAGS) $(DLLFLAGS) $(XLDFLAGS) > +LINK_EXE = $(LINK) $(OS_LFLAGS) $(LFLAGS) $(XLDFLAGS) I assume both of these are needed? (I don't know if LINK_EXE is actually being used...) ::: security/nss/coreconf/rules.mk @@ +248,5 @@ > rm -f $@.manifest; \ > fi > endif # MSVC with manifest tool > else > $(MKPROG) -o $@ $(CFLAGS) $(OBJS) $(LDFLAGS) $(EXTRA_LIBS) $(EXTRA_SHARED_LIBS) $(OS_LIBS) Should we also add $(XLDFLAGS) after $(LDFLAGS) here? @@ +331,5 @@ > $(OBJDIR)/$(PROG_PREFIX)%$(PROG_SUFFIX): $(OBJDIR)/$(PROG_PREFIX)%$(OBJ_SUFFIX) > @$(MAKE_OBJDIR) > ifeq (,$(filter-out _WIN%,$(NS_USE_GCC)_$(OS_TARGET))) > $(MKPROG) $< -Fe$@ -link \ > + $(LDFLAGS) $(EXTRA_LIBS) $(EXTRA_SHARED_LIBS) $(OS_LIBS) $(XLDFLAGS) Can we put $(XLDFLAGS) immediately after $(LDFLAGS) here?

(In reply to Wan-Teh Chang from comment #4) > Comment on attachment 8505176 [details] [diff] [review] > Patch (v1) > > Review of attachment 8505176 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=wtc. I have some questions/suggested changes. Thanks for the review! > ::: security/nss/coreconf/command.mk > @@ +11,5 @@ > > AS = $(CC) > > ASFLAGS += $(CFLAGS) > > CCF = $(CC) $(CFLAGS) > > +LINK_DLL = $(LINK) $(OS_DLLFLAGS) $(DLLFLAGS) $(XLDFLAGS) > > +LINK_EXE = $(LINK) $(OS_LFLAGS) $(LFLAGS) $(XLDFLAGS) > > I assume both of these are needed? (I don't know if LINK_EXE is actually > being used...) Hmm, yeah LINK_EXE is unused. Would you like me to remove it completely? > ::: security/nss/coreconf/rules.mk > @@ +248,5 @@ > > rm -f $@.manifest; \ > > fi > > endif # MSVC with manifest tool > > else > > $(MKPROG) -o $@ $(CFLAGS) $(OBJS) $(LDFLAGS) $(EXTRA_LIBS) $(EXTRA_SHARED_LIBS) $(OS_LIBS) > > Should we also add $(XLDFLAGS) after $(LDFLAGS) here? I can do that, but it's not necessary for my purposes (which is ASAN builds on Windows.) (BTW, NSS might be interested in this work as well, if you're not benefiting from it from Chromium already.) > @@ +331,5 @@ > > $(OBJDIR)/$(PROG_PREFIX)%$(PROG_SUFFIX): $(OBJDIR)/$(PROG_PREFIX)%$(OBJ_SUFFIX) > > @$(MAKE_OBJDIR) > > ifeq (,$(filter-out _WIN%,$(NS_USE_GCC)_$(OS_TARGET))) > > $(MKPROG) $< -Fe$@ -link \ > > + $(LDFLAGS) $(EXTRA_LIBS) $(EXTRA_SHARED_LIBS) $(OS_LIBS) $(XLDFLAGS) > > Can we put $(XLDFLAGS) immediately after $(LDFLAGS) here? Yes. I'll prepare another patch to address all of these, but I'll wait for your response to the above questions first.

Ehsan: please remove LINK_EXE completely. I think it would be nice to always use the $(LDFLAGS) $(EXTRA_LIBS) sequence, for consistency. Do you know how to do ASAN builds on Unix? Maybe we are passing the -fsanitize=address flag as part of the CC and CXX variables?

(In reply to Wan-Teh Chang from comment #6) > Ehsan: please remove LINK_EXE completely. I think it would be nice to > always use the $(LDFLAGS) $(EXTRA_LIBS) sequence, for consistency. Sure, will do. > Do you know how to do ASAN builds on Unix? Maybe we are passing the > -fsanitize=address flag as part of the CC and CXX variables? Yes, but on Windows we need to link through LD directly, so we need to be able to pass the library names to it.

This is needed for AddressSanitizer support on Windows, where we need to pass in the ASAN runtime library names from the external Mozilla build system.

Comment on attachment 8550823 [details] [diff] [review] Add support for passing external LDFLAGS r=wtc. I edited the commit message slightly and checked this in. https://hg.mozilla.org/projects/nss/rev/047f9c1d1b39

Ehsan: We expect to push this patch to mozilla-central by this Friday. If you need it in mozilla-central sooner, please let me and :kaie know.

I can wait until Friday. Thanks!

mass change target milestone to 3.17.4