Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mach-O check for improperly nested .cfi_* regions doesn't take .alt_entry into account #97116

Closed
filipnavara opened this issue Jun 28, 2024 · 5 comments · Fixed by #97479
Closed
Assignees
Labels
llvm:asmparser mc Machine (object) code

Comments

@filipnavara
Copy link

filipnavara commented Jun 28, 2024

This is a similar case to #82261 where an exception was added to for .alt_entry to the non-private labels cannot appear between .cfi_startproc / .cfi_endproc pairs check.

Commit 0b06727 introduced the invalid CFI advance_loc expression error with seemingly similar intentions.

Now, let's look at this example code:

.section __TEXT,__text
.globl _foo
_foo:
  .cfi_startproc
  sub sp, sp, 8
  .cfi_adjust_cfa_offset 8
  .alt_entry _bar
_bar:
  add sp, sp, 8
  .cfi_adjust_cfa_offset -8
  ret
  .cfi_endproc

My assumption is that .alt_entry should be exempt from the "advance_loc" check just like it's exempt from the "cfi_startproc/cfi_endproc" check. Feel free to correct my expectations if they are wrong.

As for the real world use case, this came up as an issue in the Microsoft .NET Runtime. There are some functions written in assembly that may trigger an AV at specified code points. The intention of the labels inside those functions is to mark the address of the instruction that triggers the AV so a signal handler can process it appropriately. It's not a callable entrypoint, just a way to get address of a given instruction.

@filipnavara
Copy link
Author

filipnavara commented Jun 30, 2024

/cc @MaskRay since you originally authored the commit.
/cc @jroelofs for validation on what is the intended behavior.

@EugeneZelenko EugeneZelenko added llvm:asmparser mc Machine (object) code and removed new issue labels Jun 30, 2024
@jroelofs
Copy link
Contributor

jroelofs commented Jul 1, 2024

The intent, AFAIU, is to allow this, but I'll defer to Maskray on the advance_loc bit.

The problematic case is when you have multiple independent functions that span a single cfi region when .subsections_via_symbols is present: that can cause some relocations to be manifest that can't be materialized, since they're relative to two independent subsections. .alt_entry's don't have that problem.

@MaskRay
Copy link
Member

MaskRay commented Jul 2, 2024

The intent, AFAIU, is to allow this, but I'll defer to Maskray on the advance_loc bit.

The problematic case is when you have multiple independent functions that span a single cfi region when .subsections_via_symbols is present: that can cause some relocations to be manifest that can't be materialized, since they're relative to two independent subsections. .alt_entry's don't have that problem.

I agree that the example should be supported. The bug resides in our setAtom code in MCMachOStreamer::finishImpl. I am writing a patch.

@MaskRay
Copy link
Member

MaskRay commented Jul 2, 2024

The intent, AFAIU, is to allow this, but I'll defer to Maskray on the advance_loc bit.
The problematic case is when you have multiple independent functions that span a single cfi region when .subsections_via_symbols is present: that can cause some relocations to be manifest that can't be materialized, since they're relative to two independent subsections. .alt_entry's don't have that problem.

I agree that the example should be supported. The bug resides in our setAtom code in MCMachOStreamer::finishImpl. I am writing a patch.

Patch: #97479

@filipnavara
Copy link
Author

Thanks!

@MaskRay MaskRay closed this as completed in 21276fd Jul 2, 2024
lravenclaw pushed a commit to lravenclaw/llvm-project that referenced this issue Jul 3, 2024
The current `setAtom` is inaccurate: a `.alt_entry` label can also be
recognized as an atom. This is mostly benign, but might cause two
locations only separated by an `.alt_entry` to have different atoms.

https://reviews.llvm.org/D153167 changed a `evaluateKnownAbsolute` to
`evaluateAsAbsolute` and would not fold `A-B` even if they are only
separated by a `.alt_entry` label, leading to a spurious error
`invalid CFI advance_loc expression`.

The fix is similar to llvm#82268: add a special case for `.alt_entry`.

Fix llvm#97116

Pull Request: llvm#97479
kbluck pushed a commit to kbluck/llvm-project that referenced this issue Jul 6, 2024
The current `setAtom` is inaccurate: a `.alt_entry` label can also be
recognized as an atom. This is mostly benign, but might cause two
locations only separated by an `.alt_entry` to have different atoms.

https://reviews.llvm.org/D153167 changed a `evaluateKnownAbsolute` to
`evaluateAsAbsolute` and would not fold `A-B` even if they are only
separated by a `.alt_entry` label, leading to a spurious error
`invalid CFI advance_loc expression`.

The fix is similar to llvm#82268: add a special case for `.alt_entry`.

Fix llvm#97116

Pull Request: llvm#97479
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
llvm:asmparser mc Machine (object) code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants