Sorry, but I'm not familiar with x86.

Thanks.

In D51230#1212854, @pcc wrote:

LGTM

I think this is the way that you are supposed to do it. It's the way that we're doing it elsewhere.
http://llvm-cs.pcc.me.uk/?q=createMCInstrAnalysis

I uploaded https://reviews.llvm.org/D51230 to try to address that. Could you take a look at it?

Friendly ping.

Friendly ping. Are there any more comments?

Any other comments?

Friendly ping.

Friendly ping.

Added newline to end of tests.

Added __cfi_slowpath_diag to the list of functions that trap on CFI violations.

I've rolled this into https://reviews.llvm.org/D49383.

Updated to use StringSaver. I'm still waiting on the discussion about whether or not to use dynamicEntries().

That bug should be fixed by https://reviews.llvm.org/rL338902.

No problem, and thanks for the review!

Friendly ping. This also fixes https://bugs.llvm.org/show_bug.cgi?id=38438.

That bug is fixed by https://reviews.llvm.org/D49946, which is awaiting review.

Is there a better way to handle appending "@plt" to the name? I had to add a vector to keep the original string around so that the StringRefs are still good, but that's a bit ugly.

I'm not at all familiar with ELF or parsing x86 and ARM, so I'd appreciate feedback on how this works and if there's a way to do it more cleanly (especially the parsing).

Updated to use the new scheme suggested by pcc and eugenis, which lets other parts of LVLM (e.g., objdump) use the PLT information.

In D49675#1183229, @vlad.tsyrklevich wrote:

Remind me, was there a reason why you wanted to use binary test inputs instead of assembling them?

In D49946#1179935, @Ka-Ka wrote:

I found another reproducer for the problem. This one is slightly different where a different assert blow, but I guess its only a different symptom for the same fault. As this one is slightly smaller you might want to use this one as a testcase instead (or use both)?

I can commit it.

Thanks for the explanations! pcc, that example API was very helpful. I have the core part of it working, including integration with cfi-verify and objdump. I'll finish it up and put the patches up for review at some point.

On AArch64, searching for adrp/ldr pairs is still a heuristic; is there not a more principled way of doing it? What about x86? And how do we get the name of the function this way?

In D49383#1175738, @pcc wrote:

I agree with eugenis that the PLT symbolization should live in lib/Object. That would also help us implement PLT symbolization in llvm-objdump. I was imagining that there would be a function that would take an object file and return a list of (PLT entry address, symbol) pairs. That would seem to be sufficient for llvm-objdump as well as for this code.

I was discussing PLT symbolization offline with eugenis and did a little more investigation. It turns out that it shouldn't be that hard to symbolize the PLT, at least not on aarch64. All of bfd/gold/lld start their PLT entries with a straightforward ADRP/LDR sequence, which you can pretty straighforwardly map onto a GOT entry and then onto a symbol.

Note that this could be combined with its parent patch, as that is now about supporting cross-DSO.

I replaced the command-line flag to specify functions with a hardcoded list of __cfi_slowpath, abort, and their @plt variants. Does that seem reasonable?

Thanks for the comments!

I just committed https://reviews.llvm.org/rL337830, which solves this problem the way eugenis suggested.

Added a testcase that goes one off the end of the loop counting up (and the previous update added one that counts down one off the end).

In D49602#1170109, @eugenis wrote:

Do the tests cover cases where only one or two of the checks can be omitted, but not all of them? Does not look that way.

Consider a loop where offset is always inbounds, but offset + access size may be not. Bounds checks is still needed, but it should be cheaper, because Size <= Offset will be constant false.

In D49602#1170059, @lebedev.ri wrote:

Out of general curiosity, why is this needed?
The optimization passes won't be able to/are not allowed to drop the extra unneeded checks?

In D49602#1169952, @eugenis wrote:

Do the tests cover cases where only one or two of the checks can be omitted, but not all of them? Does not look that way.

I'm not very familiar with SCEV, so I'm not sure if I'm making the correct signed vs. unsigned ranges and comparisons here. What is the proper way to do that?

Good call; I had figured that running it earlier might just impede other optimizations, but I forgot that it would also hide size information.

In D49492#1167064, @efriedma wrote:

Are you sure this will actually do what you want, in general? I suspect it will end up missing bounds checks in some cases because it's running it too early (before mem2reg/inlining/etc).

Thanks!

I committed this fix to fix the failing tests.

Updated a comment.

Thanks. I'll wait a bit to see if there are any more comments.

I believe this addresses all of the existing comments except for how to handle detecting traps. I see Vlad just commented that he doesn't have an opinion, so if there are no further opinions I'll just play around and do what I feel is best. The current approach should still work, of course.

Thanks for the comments! I'll try to update the patch on Friday.

Sorry for the trouble. I see it's been fixed now.