In D144232#4137367, @dim wrote:

In D144232#4136787, @brad wrote:

I noticed this review. I have provided a more complete diff for review at D144321.

Yeah I think that is probably the better option, hope @pkubaj and @adalava agree with that?

As Brad's version covers both FreeBSD and OpenBSD, and also updates a bunch of unit tests, which this review appears to break (see the Unit Tests https://reviews.llvm.org/harbormaster/unit/214420/).

Nice catch!
I'm not a LLVM maintainer but from FreeBSD PowerPC side I agree with the changes, only added two suggestions that could improve code readability

sorry for the long delay, I'm not professionally working on PPC64 since then.

Update comments

In D133745#3785544, @MaskRay wrote:

I made a mistake in the commit message. It should be "if ifunc"... You can verify that hasDirectReloc (now HAS_DIRECT_RELOC) is only functional when isGnuIfunc() is true.

In D133745#3785479, @MaskRay wrote:

This is incorrect. HAS_DIRECT_RELOC is properly not named clearly but it intends to refer to just STT_GNU_IFUNC symbols.
Changing the comparison will break a lot of non-preemptible ifunc tests.

In D116867#3432592, @pkubaj wrote:

Removing kcc from reviewers since he seems inactive for the part half a year.
@adalava
As this is now accepted, can you commit it and if possible (since this is a build fix) merge for 14.0.1?

It's not actually a problem but tb_freq should be initialized as integer. Otherwise looks good to me!

LGTM, Tested on FreeBSD/powerpc 32 bit

long long (8-bytes) can be lock free on PPC64, but it's not lock free on PPC32.

In D113443#3147470, @MaskRay wrote:

readelf -r without -W may not show the full relocation type name.
The relocation type is R_X86_64_REX_GOTPCRELX, not R_X86_64_REX_GOTP.

As I mentioned, -fno-pic may need GOT-generating relocations as well to avoid copy relocations. Not handling R_X86_64_REX_GOTPCRELX is FreeBSD's limitation which should be fixed https://maskray.me/blog/2021-08-29-all-about-global-offset-table

I haven't read all the PowerPC discussions but it seems that there may be a dso_local PowerPC64 ELFv2 issue as well which should be properly fixed.

With all these said, I am fine that this is pushed to fix the immediate problem for FreeBSD. But if FreeBSD 14.0.0 plans to fix R_X86_64_REX_GOTPCRELX, this patch is probably not very necessary.

The issue could also be avoided by defaulting to -mstack-protector-guard=tls for FreeBSD, like Linux.

In D113451#3124461, @jsji wrote:

LGTM. Thanks for enabling this for FreeBSD.

fix style

add more ppc_wrappers

In D109090#3113552, @dim wrote:

Hmm this seems to cause some trouble when building kernels and kernel modules on FreeBSD, which are built in -ffreestanding mode, and then you get weird relocations in kernel modules:

• Simplified register number check as suggested by @MaskRay
• Added test for powerpcle (32 bits LE)

@sfertile is this last version still good for you? (after removing isPPC64 from if clause)

add tests for FreeBSD

It's now avoiding a copy relocation for '__stack_chk_guard':

Update patch to avoid setting dso_local for any FreeBSD target since symbol is defined on libc.so.7.

In D109090#3007009, @MaskRay wrote:

I am afraid the root cause is still unknown and this patch just papers over the issue.
I hope we have a good understanding why having dso_local will cause trouble on FreeBSD powerpc64's -fstack-protector* and fix the root cause instead.

In D109090#2978298, @arichardson wrote:

This does not seem like the right approach to me. Have you looked into why the pointer is not correct. Does it affect static binaries or dynamic ones? Maybe this can be fixed in rtld instead? As it works for all other architectures (and PPC Linux), I feel like that is the more likely cause

rebased from stable/13.x to main branch

This was already commited, but for the record , I tested it on FreeBSD 13/powerpc64

I confirmed it fixes the case Piotr Kubaj reported. Thank you!

In D71600#1868284, @efriedma wrote:

On master, atomic.c is not built by default. It's only built if you explicitly request it with something like the CMake flag -DCOMPILER_RT_EXCLUDE_ATOMIC_BUILTIN=Off . If you're not doing that, any change to atomic.c has no effect.

In D71600#1867135, @efriedma wrote:

For the clang change, we should do something like D72579, not explicitly check for a specific target in target-independent code.

fix typo found by @dim

Updated comment

In D71600#1816160, @MaskRay wrote:

I am still confused why you need the special rule for __atomic_is_lock_free (GCC/clang) and __c11_atomic_is_lock_free (clang).

https://github.com/gcc-mirror/gcc/blob/master/gcc/builtins.c#L7300-L7314 GCC __atomic_is_lock_free expands to either 1 or a library call to __atomic_is_lock_free, never 0. How did FreeBSD get around libatomic when it was still using GCC? In clang, We can probably extend the semantics of __c11_atomic_is_lock_free and use clang::TargetInfo::getMaxAtomicPromoteWidth, but I'll make more research in this area.

Can you be elaborate how do __atomic_is_lock_free/__c11_atomic_is_lock_free pull in libatomic.{a,so} dependency on FreeBSD powerpc?

In D72306#1808613, @MaskRay wrote:

Obsoleted by D72352 ?

LGTM. Just a comment regarding FreeBSD 11.x.

In D72306#1807097, @MaskRay wrote:

Are you still using -target powerpc64-unknown-freebsd12.0-elfv2 or -target powerpc64-unknown-freebsd12.0-elfv1 added in https://reviews.llvm.org/rL361355 ?

As I'm not a commiter, may someone commit it, if it's the case, please?

update fixing the typo found during review.

In D67190#1670325, @jdoerfert wrote:

Is there a good way to test this? If so, we should, if not, this seems fine to me (except the typo @hbae found).

This solution works for me.
Validated it on FreeBSD13-CURRENT/powerpc64, with 32 bit compatibility libraries built using this patch over LLVM9.

Great job, @MaskRay !!

In D61950#1504169, @MaskRay wrote:

Will FreeBSD 13 or future releases support ELFv1? If not, it may be cleaner to not invent -elfv2 -elfv1 triples, but rather dispatch on the major version, e.g. powerpc64-unknown-freebsd13.0 could mean ELFv2.

• remove redundant comment

In D61610#1496129, @MaskRay wrote:

@adalava We care about the link time. I need to do some testing to ensure this doesn't cause regression. I've created https://reviews.llvm.org/D61720, fixed the tests and added ppc64-long-branch-init.s that I think captures the idea of the fix. Hope you don't mind my commandeering the change :)

In D61647#1496067, @MaskRay wrote:

See https://reviews.llvm.org/D61718 , not many tests need updating. The dump still diverges from GNU objdump in a few ways, e.g. the width of the size field, the number of spaces, the 'g' symbol etc

update testcase to use readelf instead of objdump, so it doesn't depend on D61647 anymore

In D61647#1494441, @MaskRay wrote:

This change makes sense and it will improve compatibility with GNU objdump. For your tests (I've commented there), I usually prefer llvm-readelf/llvm-readobj to llvm-objdump. The output of objdump also looks very weird to me. I remember @emaste said so in one FreeBSD issue.. I can check how many tests (you should at least check ninja -C Release check-llvm check-lld; clang has <5 llvm-objdump tests; ) will be impacted by this change. If there aren't many, I can fix them and commit that for you :)

In D59436#1494416, @MaskRay wrote:

Thanks. The test contains some constructs that are not used, e.g. .symver .addrsig. And I think it is usually better to use llvm-readelf/llvm-readobj. I always find the output of llvm-objdump/objdump weird.. If you don't mind, I can rewrite/minimalize the test and commit for you.

increase to PPC64_REL24 range

In D61647#1494070, @sfertile wrote:

This definitely seems a useful upgrade to me. Unfortunately it might affect a lot of lit tests. If we do intend to make a change we should try to match the output of the binutils objdump. I can see from the surrounding context we print out '.hidden', having a quick look it seems binutils will print out the visibility if its the only thing set, and the hex value otherwise (ie not both) here is an example:

Thank you @MaskRay, a test case was added. Please note this patch currently depends on D56586 and D61647 that need to go in first.

add test case

Thank you guys. I updated the patch,;

fix patch

• add space reservation, add comments and improve test

In rG70f25ebc6d1588bdf7fa07e7c42fa3c9a075101d#639599, @lebedev.ri wrote:

It was brought up before.
phab for some reason counts all the commits to all the github forks of the official github llvm monorepo like this

Sorry, I was in the wrong repository! It was meant to be in my private one.

Example code: