Clearing UpdateOther in finish() seems correct.
I've not considered the reuse of PPCTargetELFStreamer objects in the .localentry fix.

LGTM. It seems I did not consider that Symbol could change its type between when it was added in emitAssignment() and when processed in finish().

Nice, I've confirmed that it fixes PR42759 on FreeBSD.

@MaskRay, this change should be good to go now, the last revision should address your last comments.

Thanks for the reviews @compnerd, @nemanjai, and @mclow.lists.
As I don't have permission to commit this to LLVM repository, can anyone commit this change for me?

In D61792#1500285, @mclow.lists wrote:

As far as I can tell, this should have no effects - is that correct?
If so, this looks fine to me.

Thanks for the review @MaskRay.
I've moved the test case to ppc64-localentry-symver.s, but then I thought it was better to rename it to ppc64-localentry-symbols.s, as now the test scope is broader than .symver testing.

• Addressed MaskRay review comments

Any update on this?

Any update on this?

• [PPC64] Update LocalEntry from assigned symbols
• Addressed review's comments
• Added a test case for the fix

In D59694#1476888, @nemanjai wrote:

Sorry, could you please rebase to latest? It does not apply cleanly for me (assembly.h does not apply).

Rebasing with latest master

@nemanjai, I've included a test case in the last revision. Do you think this is ok now?
Also, if there are no further issues, could you please check this in for me?

• Test libunwind exception handling fix for PPC64

Sorry for the delay, I ran into some issues.
This change addresses the review comments.
It also fixes assembly functions when targeting PowerPC64 with the ELFv1 ABI, as it requires an entry in .opd section for each function.
The compiler usually does this, but it must be inserted by hand for assembly functions.

• Addressed review comments
• Fixed ELFv1 ASM functions

Thank you @ruiu. Can you commit this change for me?

In D56782#1394013, @ruiu wrote:

Sorry for the belated response. I missed this one for some reason.

Can you add a test?

• Addressed review's comments
• Added a test for the fix

In D56586#1354013, @labath wrote:

I suggest having this reviewed by the PPC target owner in llvm.

@clayborg, it seems this change ended up forgotten here. Can you check this in when you have the time? Thanks.

ping

Is there anything pending on this yet?
It seems that we just need someone with write access to check in the changes, right?

Thanks @clayborg! Can you please commit this change for me?

• Changed parameter: Symbol -> SymbolContext

@lbianc is no longer working on this. I'll continue his work from where he left it.

In D44437#1219616, @clayborg wrote:

In D44437#1219196, @luporl wrote:

Hello @clayborg, any update on this?

Is it clear why, at least on PPC64, the SymbolContext is not needed, as the needed info is at the symbol, and only at the symbol?

It is fine for PPC64 to only require a symbol, but for other architectures, it might be able to use the function. Since we are designing APIs for all architectures here, we should send an entire symbol context just in case. It is fine for the PPC64 version to require just the symbol. but it should be grabbed from the symbol context

Hello @clayborg, any update on this?

Any update on this?
It seems it only need Greg or Jim to take a look at the final result, right?

Thanks @labath.
Could you please commit this patch?

Closed by commit rL327331: Re-add change for https://reviews.llvm.org/D42582 with added directories.

Thanks @labath. Can you check this in for me?

I don't know much about this special PPC64 key. I just noticed that it is used multiple times in PPC's auxvec, which breaks current test's logic.

• Added description about ignored PPC auxvec key

@labath, @jasonmolenda, any updates on this? Can we just change the assert for now?

@labath, @clayborg, can you commit this change for me?

Thank you @labath, @clayborg and @jingham. Can you please check this in for me?

Thank you @labath. Can you check this in for me?

Thank you @clayborg. Can you check this in for me?

@jingham, I think this needs your approval to go ahead, right?

• Merge branch 'master' into fix-step-in
• Improved ppc64 local entry point test

@labath, did you have a chance to look at the breakpoint test, and the new "where" line in lldb-test?

@clayborg, do you think it is ok now?

• Merge branch 'master' into fix-tests-brkpts

@labath, @clayborg, can you check this in for me?

BTW, why is failure expected in i386?

In D44022#1026098, @jasonmolenda wrote:

Pavel raises a good point, but I don't know if the onerous should be on this patch author to work out a better way to test this.

The question I have is -- what is the target triple on your linux system? Is the vendor field of the triple empty? To be honest, we could test for .startswith('x86_64-') here and that's all we really need to be doing. If the host platform is "infecting" the triple with a vendor or OS, that's not what the test is intended to check.

• Merge branch 'master' into fix-tests-brkpts
• Forcing PPC64 to use function global entry point

• Merge branch 'master' into fix-auxv-test
• Addressed review's comments

Thanks @labath. Can you check this in for me?

• Merge branch 'master' into fix-step-in
• Added break in function local entry point test

In D43768#1019395, @labath wrote:

I think we should find some other way to fix this. Is there a way to force the inferior to go through the global entry point? Would putting the function in a different compilation unit help? Or calling it through a function pointer?

• Improved GetBytesToSkip() interface
• Performed requested changes

In D42582#1018105, @jingham wrote:

GetBytesToSkip strikes me as an odd function. It takes a thread but implicitly acts on the PC of frame 0 of that thread. That makes it feel overly specific and requires a process when you don't really need one to do the calculation. Plus you already have the pc & symbol but you end up re-looking them up. You could do all the calculations you do in GetBytesToSkip if you passed in the symbol and the pc as an Address. That would allow you to get the file address for the PC, and you can compare that directly to the symbol's address.

You can get an Address from a load address (which curr_addr is) by calling target->GetSectionLoadList()->ResolveLoadAddress(curr_addr, ReturnedAddress). Then you can use the file address from the ReturnedAddress to compare against the FileAddress of the symbol.

The way you are using these is fine, BTW. I'm just quibbling about the interface of GetBytesToSkip.

In D42582#1017455, @labath wrote:

I don't think I know enough about this, so I'll defer to Jim to comment on the design. I just have some comments about the implementation.

Also, it looks like at least the breakpoint-setting part should be easy to test in a freestanding environment: make a .s file which sets up the function, llvm-mc it, load it up in lldb, set a breakpoint, verify it got moved to the right offset. I've been playing around with a patch that should make this easier for you. I'll see if I can clean it up and upload today...

• Restored PPC64 Architecture Plugin
• Restored/refactored GetBytesToSkip() method, to present a cleaner API (this replaces the trampoline code attempt, that didn't work very well)
• Added an AdjustBreakpointAddress method, to fix issues with setting breakpoints in functions with multiple entry points

Thanks @clayborg and @labath! Can you please commit this patch for me?

• Revert "Changed GetBytesToSkip comment to focus on the API"
• Revert "Fixed ArchitecturePPC64 initialization"
• Revert "Moved changes to new ArchitecturePPC64 plugin"
• Revert "Fixed step-in stopping in the wrong line in PPC64"
• Handling PPC64 local entry point with extra symbol
• Merge branch 'master' into fix-step-in2

• Changed the plugin to not store the Log pointer

• Changed debug printfs to log messages

In D43345#1012153, @labath wrote:

How would you feel about replacing the custom debug printfs with standard logging messages?

That way, you can always have them available without recompiling (and it's always nice to have more uniformity).

• Merge branch 'master' into ppc64-inst-emu2
• Addressed review comments

This FileCheck-style test seems to be easier to write and understand.
In fact, I wrote a gtest-style test because:
1- I didn't know I could write a FileCheck-style test instead.
2- This was inspired by the ARM64 plugin, that seemed like the simplest one to understand and follow.