Page MenuHomeFreeBSD

Avoid adding duplicates to SRCS/OBJS/SOBJS/POBJS
ClosedPublic

Authored by arichardson on Aug 12 2020, 5:09 PM.

Details

Summary

This is a change in preparation for stopping to use lorder.sh (D26044) and
instead assume that we have a linker new than ~1990. Without lorder
duplicates end up being passed to the linker when building .so files and this
results in duplicate symbol definition errors otherwise.

There is one minor change: libcompiler_rt.a will no longer provide
gcc_personality_v0 and instead we now only have it in libgcc_eh.a/libgcc_s.so.
This matches GCC's behaviour.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lib/libgcc_s/Makefile
56 ↗(On Diff #75737)

Where do the duplicates come from? Perhaps we could just avoid adding the dups in the first place, although perhaps that's awkward and this is fine.

lib/libprocstat/Makefile
61 ↗(On Diff #75737)

SOBJS/POBJS/ originally came in from rS221807, maybe just due to a misunderstanding. Normally only OBJS is set, so I'd argue the comment here is unnecessary and is probably more confusing to a future code archaeologist - I think it can just be deleted.

stand/userboot/userboot/Makefile
23 ↗(On Diff #75737)

Is this just unused?

lib/libgcc_s/Makefile
56 ↗(On Diff #75737)

The two include lines above seem to add duplicates. I think it might be a bit awkward to remove the duplicates but I can look into that tomorrow.

.include "../libcompiler_rt/Makefile.inc"
.include "../libgcc_eh/Makefile.inc"
lib/libprocstat/Makefile
61 ↗(On Diff #75737)

Will fix.

stand/userboot/userboot/Makefile
23 ↗(On Diff #75737)

It's added below in the .include "${BOOTSRC}/loader.mk" line.

emaste added inline comments.
lib/libgcc_s/Makefile
56 ↗(On Diff #75737)

OK - then I think it's reasonable to commit this one for now, and we can later look at avoiding the duplicates and removing this again.

stand/userboot/userboot/Makefile
23 ↗(On Diff #75737)

OK

This revision is now accepted and ready to land.Aug 12 2020, 6:20 PM
arichardson added inline comments.
lib/libgcc_s/Makefile
56 ↗(On Diff #75737)

Seems like the problem is gcc_personality_v0 which is included in both the compiler_rt and libgcc_eh makefile.inc.

@dim does it need to be in both?

It seems like on Linux libgcc (i.e. libcompiler_rt) doesn't provide it, only libgcc_eh (and libgcc_s.so.1) include it.

/usr/lib/gcc/x86_64-linux-gnu/7/libgcc.a:morestack.o: 0000000000000000 V DW.ref.__gcc_personality_v0
/usr/lib/gcc/x86_64-linux-gnu/7/libgcc.a:morestack.o:                  U __gcc_personality_v0
/usr/lib/gcc/x86_64-linux-gnu/7/libgcc_eh.a:unwind-c.o: 00000000000001b0 T __gcc_personality_v0

Doing some archaeology, it seems I moved gcc_personality_v0 to lib/libcompiler_rt/Makefile.inc (from lib/libcompiler_rt/Makefile) in rS306377, added to lib/libgcc_eh/Makefile.inc in rS307230 (D8188).

This revision now requires review to proceed.Aug 13 2020, 2:17 PM
This revision is now accepted and ready to land.Aug 13 2020, 3:42 PM
dim added inline comments.
lib/libgcc_s/Makefile
56 ↗(On Diff #75737)

Good question. A very long time ago in rS276851, I added compiler-rt trunk r276851, and at that point added the gcc_personality_v0.c file to what was then lib/libcompiler_rt/Makefile. (This got renamed to Makefile.inc later.)

Then in rS307230, @emaste added the scaffolding Makefiles for libgcc_eh and libgcc_s based on compiler-rt, and this initial version also contained an entry for gcc_personality_v0.c.

I am unsure which library is *supposed* to provide it, though. Upstream compiler-rt places it in its lib/builtins directory, which typically goes in to libcompiler_rt.a.

Note that gcc_personality_v0.c has a comment added by @cem in rS354347, related to this:

* XXX On FreeBSD, this file is compiled into three libraries:
*   - libcompiler_rt
*   - libgcc_eh
*   - libgcc_s
*
* In the former, the include path points to the contrib/libcxxrt/unwind-arm.h
* copy of unwind.h.  In the latter, the include path points to the
* contrib/libunwind/include/unwind.h header (LLVM libunwind).
*
* Neither (seemingly redundant) variant of unwind.h needs the redefinitions
* provided in the "helpful" header below, and libcxxrt's unwind-arm.h provides
* *no* useful distinguishing macros, so just forcibly disable the helper
* header on FreeBSD.
*/

So this may indeed be some historical mistake. I'd say it only belongs in libcompiler_rt proper, that is, libcompiler_rt.a and libgcc_s.so, and not in libgcc_eh.a. But it is possible that it must also be available in libgcc_eh.a for compatibility reasons.

Looking at what gcc does, from its build script and Makefiles, I infer that they only put the personality functions in libgcc_eh.a and libgcc_s.so, *not* in libgcc.a.

This is also what I see in actual installed binaries, for example from the gcc9 port:

% grep -R gcc_personality /usr/local/lib/gcc9
Binary file /usr/local/lib/gcc9/libgcc_s.so matches
Binary file /usr/local/lib/gcc9/gcc/x86_64-portbld-freebsd12.1/9.3.0/32/libgcc_eh.a matches
Binary file /usr/local/lib/gcc9/gcc/x86_64-portbld-freebsd12.1/9.3.0/libgcc_eh.a matches
Binary file /usr/local/lib/gcc9/libgcc_s.so.1 matches

E.g., I think we can remove the duplicate copy from libcompiler_rt.a safely, and ensure that gcc_personality_v0.c is only added to libgcc_eh.a and libgcc_s.so.

In D26042#578249, @dim wrote:

E.g., I think we can remove the duplicate copy from libcompiler_rt.a safely, and ensure that gcc_personality_v0.c is only added to libgcc_eh.a and libgcc_s.so.

Sounds good to me.

lib/libgcc_s/Makefile
56 ↗(On Diff #75737)

If we remove gcc_personality_v0 from libcompiler_rt, can this SRCS deduplication line be removed?

lib/libgcc_s/Makefile
56 ↗(On Diff #75737)

If we remove gcc_personality_v0 from libcompiler_rt, can this SRCS deduplication line be removed?

Yes I believe that was the only duplicate. Will check shortly.

  • Remove duplicates in libgcc_s without uniq:
    • Remove gcc_personality from libcompiler_rt
    • move int_util.c from libgcc_eh/Makefile.inc to Makefile
This revision now requires review to proceed.Aug 14 2020, 10:03 AM

Thanks for doing this! I’ve been really happy to see all your bootstrap portability changes streaming in recently.

lib/libgcc_eh/Makefile
11 ↗(On Diff #75818)

I’m not sure this needs a comment. Makefile.inc is the weird case, srcs in Makefile is normal. The commit log for this line will justify it for anyone especially curious.

This revision is now accepted and ready to land.Aug 14 2020, 3:53 PM