1082973 - Add support for passing external LDFLAGS

Status: RESOLVED FIXED

(NSS :: Build, defect, P1)

Description

[(no longer active)]

Attachment: Patch (v1)

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 1

[(no longer active)]

ping?

Comment 2

[(no longer active)]

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 3

[Richard Barnes [:rbarnes]]

Comment on attachment 8505176 [details] [diff] [review]
Patch (v1)

Brian and David aren't NSS peers.

Comment 4

[Wan-Teh Chang]

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?

Comment 5

[(no longer active)]

(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.

Comment 6

[Wan-Teh Chang]

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?

Comment 7

[(no longer active)]

(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.

Comment 8

[(no longer active)]

Attachment: Add support for passing external LDFLAGS

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 9

[Wan-Teh Chang]

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

Comment 10

[Wan-Teh Chang]

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.

Comment 11

[(no longer active)]

I can wait until Friday.  Thanks!

Comment 12

[Kai Engert (:KaiE:)]

mass change target milestone to 3.17.4