[v4,00/15] sframe: Enhancements to SFrame info generation

  Patches 1 and 2 (updated) are minor cleanups/enhancements to the
existing SFrame support on AArch64 and x86 AMD64.

Patch 3 enables readelf/objdump to dump the SFrame fixed offsets from
CFA to the frame pointer (FP) and return address (RA).

Patch 4 changes readelf/objdump to display 'f' in the SFrame
RA tracking column, if the architecture is using a fixed RA offset.
Additionally it corrects the logic to display 'u' in the SFrame
FP tracking column.

Patch 5 (updated) enhances an SFrame warning message to print the human
readable DWARF call frame instruction name.

Patches 6 and 7 resolve issues that cause the assembler to either
generate bad SFrame FDE or to silently skip it. Both issues would be
triggered by s390-specific SFrame error test cases introduced by the
separate patch series.

Patch 8 refactors SFrame CFI opcode DW_CFA_register processing into a
separate function. This harmonizes the CFI opcode processing.

Patch 9 (updated) adds verbose assembler warning messages when
generation of SFrame FDE is skipped.

Patch 10 (updated) resolves an issue that causes the assembler to
generate bad SFrame FDE in case the FP without RA was saved on the
stack, which the SFrame format cannot represent. I will send two
alternative solution proposals as RFC.

Patch 11 (updated) skips SFrame FDE for .cfi_window_save on all
architectures except AArch64, which multiplexed it with
.cfi_negate_ra_state.

Patches 12 and 13 (updated) resolve issues where generation of SFrame
FDE was unnecessarily skipped.

Patch 14 adds tests for the SFrame RA tracking predicate to places where
it was missing to align the logic.

Patch 15 (updated) is a minor enhancement to add checks that the
architecture-dependent RA tracking is correctly configured.

Changes v3 -> v4:
- Removed dash in terms "stack pointer", "frame pointer", and "return
  address" as requested by Indu.
- Use terse warning message texts as suggested by Indu.
- use proper DWARF terminology "CFI instruction" as suggested by Indu.
- Fix bad indentation reported by GCC's check_GNU_style.py.
- Dropped misleading comment "No errors encountered". as suggtested by
  Indu.
- Add comment to sframe_xlate_do_offset why SP register is ignored.

Changes v2 -> v3:
- Additional patches as noted in cover letter and patch notes.
- Reword further SFrame macro, variable, and function descriptions
  to align with those previously touched.
- Align description of definition in source with declaration in header.
- Updated gas synthesized CFI test cases for x86 AMD64 to test for
  architecture-specific fixed RA offset instead of using a pattern.
- Updated ld SFrame test cases for x86 AMD64 to test for architecture-
  specific fixed RA offset.
- Updated SFrame warning message texts as suggested by Indu, except for
  the .cfi_def_cfa[_register] ones.
- Do not test sframe_ra_tracking_p when determining the SFrame register
  name in sframe_register_name. The RA register name does not depend on
  whether RA tracking is used or not.
- Corrected formatting of ChangeLog in commit message.

Changes v1 -> v2:
- Resolved a regression reported by Linaro-TCWG-CI on AArch64 in one of
  the generic ld SFrame test cases. The test case contained a
  .cfi_def_cfa directive, specifying a CFA base register number that is
  not necessarily a SFrame SP/FP register number on all architectures.
  This caused the changes from patch 6 to skip SFrame FDE generation on
  AArch64 (and s390x aswell) with a warning, causing the test case to
  fail.

Thanks and regards,
Jens

Jens Remus (15):
  x86: Remove unused SFrame CFI RA register variable
  gas: Enhance arch-specific SFrame configuration descriptions
  readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
  readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
  gas: Print DWARF call frame insn name in SFrame warning message
  gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
  gas: Warn if SFrame FDE is skipped due to non-default return column
  gas: Refactor SFrame CFI opcode DW_CFA_register processing
  gas: User readable warnings if SFrame FDE is not generated
  gas: Skip SFrame FDE if FP without RA on stack
  gas: Skip SFrame FDE if .cfi_window_save
  gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
  gas: Don't skip SFrame FDE if .cfi_register specifies SP register
  gas: Test predicate whether SFrame RA tracking is used
  gas: Validate SFrame RA tracking and fixed RA offset

 gas/config/tc-aarch64.c                       |   6 +-
 gas/config/tc-aarch64.h                       |  12 +-
 gas/config/tc-i386.c                          |   6 +-
 gas/config/tc-i386.h                          |  10 +-
 gas/gen-sframe.c                              | 246 +++++++++++++++---
 gas/gen-sframe.h                              |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-1.d      |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-2.d      |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-3.d      |   2 +
 .../gas/cfi-sframe/cfi-sframe-common-4.d      |   6 +-
 .../gas/cfi-sframe/cfi-sframe-common-5.d      |   6 +-
 .../gas/cfi-sframe/cfi-sframe-common-6.d      |   6 +-
 .../gas/cfi-sframe/cfi-sframe-common-7.d      |   6 +-
 .../gas/cfi-sframe/cfi-sframe-common-8.d      |   4 +-
 .../gas/cfi-sframe/cfi-sframe-x86_64-1.d      |   9 +-
 gas/testsuite/gas/cfi-sframe/common-empty-1.d |   4 +-
 gas/testsuite/gas/cfi-sframe/common-empty-2.d |   4 +-
 gas/testsuite/gas/cfi-sframe/common-empty-3.d |   3 +
 .../gas/scfi/x86_64/scfi-cfi-sections-1.d     |  11 +-
 .../gas/scfi/x86_64/scfi-dyn-stack-1.d        |  11 +-
 ld/testsuite/ld-sframe/discard.s              |   1 -
 ld/testsuite/ld-x86-64/sframe-plt-1.d         |   9 +-
 ld/testsuite/ld-x86-64/sframe-simple-1.d      |  17 +-
 libsframe/sframe-dump.c                       |  18 +-
 24 files changed, 305 insertions(+), 98 deletions(-)
  

On 24.06.2024 18:13, Jens Remus wrote:
> Am 24.06.2024 um 16:51 schrieb Jan Beulich:
>> On 24.06.2024 16:23, Jens Remus wrote:
>>> gas/
>>> 	* config/tc-i386.c: Remove unused SFrame CFI RA register
>>> 	variable.
>>
>> Nit: The more "canonical" way would be
>>
>> 	* config/tc-i386.c (x86_sframe_cfa_ra_reg): Remove.
>>
> 
> Thanks! I am still learning how to get those GNU Changelog entries in 
> the commit message correct. While this series evolved later added 
> patches do have the function names. Somehow I did not consider to reword 
> them all.
> 
> I will reword the commit message as you suggested. Do you want me to 
> send a v5 (after Indu reviewed the whole series)?

I'd recommend not waiting until the full series is ready, at least for
entirely independent patches like this one. Just put it in.

Jan
  

Hello Indu!

Am 24.06.2024 um 16:23 schrieb Jens Remus:
...
> Jens Remus (15):
>    x86: Remove unused SFrame CFI RA register variable
>    gas: Enhance arch-specific SFrame configuration descriptions
>    readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>    readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>    gas: Print DWARF call frame insn name in SFrame warning message
>    gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>    gas: Warn if SFrame FDE is skipped due to non-default return column
>    gas: Refactor SFrame CFI opcode DW_CFA_register processing
>    gas: User readable warnings if SFrame FDE is not generated
>    gas: Skip SFrame FDE if FP without RA on stack
>    gas: Skip SFrame FDE if .cfi_window_save
>    gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>    gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>    gas: Test predicate whether SFrame RA tracking is used
>    gas: Validate SFrame RA tracking and fixed RA offset...

As Jan pointed out there are issues with my patches appearing severely 
delayed on the list. Furthermore it seems patches 1, 6, and 9 did not 
make it to the list at all. I have reached out to mailman@sourceware.org 
to hopefully get this resolved.

Nevertheless you should have received the whole series since you were 
explicitly listed as recipient. I am just not sure whether it makes 
sense for you to review them, as your replies to patches 1, 6, and 9 
would appear out of nowhere on the list.

I would like to wait for a response from mailman@sourceware.org before 
attempting a resend, as this is not the first time my patches get 
delayed (although the first time some are dropped).

Hopefully my missing patches are only awaiting moderation and can be 
reinstated.

Regards,
Jens
  

Hello Indu!

Am 25.06.2024 um 10:22 schrieb Jens Remus:
> Am 24.06.2024 um 16:23 schrieb Jens Remus:
> ...
>> Jens Remus (15):
>>    x86: Remove unused SFrame CFI RA register variable
>>    gas: Enhance arch-specific SFrame configuration descriptions
>>    readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>>    readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>>    gas: Print DWARF call frame insn name in SFrame warning message
>>    gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>>    gas: Warn if SFrame FDE is skipped due to non-default return column
>>    gas: Refactor SFrame CFI opcode DW_CFA_register processing
>>    gas: User readable warnings if SFrame FDE is not generated
>>    gas: Skip SFrame FDE if FP without RA on stack
>>    gas: Skip SFrame FDE if .cfi_window_save
>>    gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>>    gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>>    gas: Test predicate whether SFrame RA tracking is used
>>    gas: Validate SFrame RA tracking and fixed RA offset...
> 
> As Jan pointed out there are issues with my patches appearing severely 
> delayed on the list. Furthermore it seems patches 1, 6, and 9 did not 
> make it to the list at all. I have reached out to mailman@sourceware.org 
> to hopefully get this resolved.
> 
> Nevertheless you should have received the whole series since you were 
> explicitly listed as recipient. I am just not sure whether it makes 
> sense for you to review them, as your replies to patches 1, 6, and 9 
> would appear out of nowhere on the list.
> 
> I would like to wait for a response from mailman@sourceware.org before 
> attempting a resend, as this is not the first time my patches get 
> delayed (although the first time some are dropped).
> 
> Hopefully my missing patches are only awaiting moderation and can be 
> reinstated.

Nick kindly assisted to get this resolved. The previously missing 
patches have meanwhile shown up in my inbox and the binutils list 
archive. From my perspective there is therefore no need for me to resent 
and you should be safe to review my patch series from yesterday.

Regards,
Jens
  

Am 24.06.2024 um 16:23 schrieb Jens Remus:
> Patches 1 and 2 (updated) are minor cleanups/enhancements to the
> existing SFrame support on AArch64 and x86 AMD64.
> 
> Patch 3 enables readelf/objdump to dump the SFrame fixed offsets from
> CFA to the frame pointer (FP) and return address (RA).
> 
> Patch 4 changes readelf/objdump to display 'f' in the SFrame
> RA tracking column, if the architecture is using a fixed RA offset.
> Additionally it corrects the logic to display 'u' in the SFrame
> FP tracking column.
> 
> Patch 5 (updated) enhances an SFrame warning message to print the human
> readable DWARF call frame instruction name.
> 
> Patches 6 and 7 resolve issues that cause the assembler to either
> generate bad SFrame FDE or to silently skip it. Both issues would be
> triggered by s390-specific SFrame error test cases introduced by the
> separate patch series.
> 
> Patch 8 refactors SFrame CFI opcode DW_CFA_register processing into a
> separate function. This harmonizes the CFI opcode processing.
> 
> Patch 9 (updated) adds verbose assembler warning messages when
> generation of SFrame FDE is skipped.
> 
> Patch 10 (updated) resolves an issue that causes the assembler to
> generate bad SFrame FDE in case the FP without RA was saved on the
> stack, which the SFrame format cannot represent. I will send two
> alternative solution proposals as RFC.
> 
> Patch 11 (updated) skips SFrame FDE for .cfi_window_save on all
> architectures except AArch64, which multiplexed it with
> .cfi_negate_ra_state.
> 
> Patches 12 and 13 (updated) resolve issues where generation of SFrame
> FDE was unnecessarily skipped.
> 
> Patch 14 adds tests for the SFrame RA tracking predicate to places where
> it was missing to align the logic.
> 
> Patch 15 (updated) is a minor enhancement to add checks that the
> architecture-dependent RA tracking is correctly configured.

...

> Jens Remus (15):
>    x86: Remove unused SFrame CFI RA register variable
>    gas: Enhance arch-specific SFrame configuration descriptions
>    readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>    readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>    gas: Print DWARF call frame insn name in SFrame warning message
>    gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>    gas: Warn if SFrame FDE is skipped due to non-default return column
>    gas: Refactor SFrame CFI opcode DW_CFA_register processing
>    gas: User readable warnings if SFrame FDE is not generated
>    gas: Skip SFrame FDE if FP without RA on stack
>    gas: Skip SFrame FDE if .cfi_window_save
>    gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>    gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>    gas: Test predicate whether SFrame RA tracking is used
>    gas: Validate SFrame RA tracking and fixed RA offset
> 
>   gas/config/tc-aarch64.c                       |   6 +-
>   gas/config/tc-aarch64.h                       |  12 +-
>   gas/config/tc-i386.c                          |   6 +-
>   gas/config/tc-i386.h                          |  10 +-
>   gas/gen-sframe.c                              | 246 +++++++++++++++---
>   gas/gen-sframe.h                              |   2 +
>   .../gas/cfi-sframe/cfi-sframe-common-1.d      |   2 +
>   .../gas/cfi-sframe/cfi-sframe-common-2.d      |   2 +
>   .../gas/cfi-sframe/cfi-sframe-common-3.d      |   2 +
>   .../gas/cfi-sframe/cfi-sframe-common-4.d      |   6 +-
>   .../gas/cfi-sframe/cfi-sframe-common-5.d      |   6 +-
>   .../gas/cfi-sframe/cfi-sframe-common-6.d      |   6 +-
>   .../gas/cfi-sframe/cfi-sframe-common-7.d      |   6 +-
>   .../gas/cfi-sframe/cfi-sframe-common-8.d      |   4 +-
>   .../gas/cfi-sframe/cfi-sframe-x86_64-1.d      |   9 +-
>   gas/testsuite/gas/cfi-sframe/common-empty-1.d |   4 +-
>   gas/testsuite/gas/cfi-sframe/common-empty-2.d |   4 +-
>   gas/testsuite/gas/cfi-sframe/common-empty-3.d |   3 +
>   .../gas/scfi/x86_64/scfi-cfi-sections-1.d     |  11 +-
>   .../gas/scfi/x86_64/scfi-dyn-stack-1.d        |  11 +-
>   ld/testsuite/ld-sframe/discard.s              |   1 -
>   ld/testsuite/ld-x86-64/sframe-plt-1.d         |   9 +-
>   ld/testsuite/ld-x86-64/sframe-simple-1.d      |  17 +-
>   libsframe/sframe-dump.c                       |  18 +-
>   24 files changed, 305 insertions(+), 98 deletions(-)

Thank you for the review, Indu!

Can I go ahead and commit the series to mainline, with the review 
feedback implemented and the offending trailers removed?

Thanks and regards,
Jens
  

On 6/27/24 01:28, Jens Remus wrote:
> Am 24.06.2024 um 16:23 schrieb Jens Remus:
>> Patches 1 and 2 (updated) are minor cleanups/enhancements to the
>> existing SFrame support on AArch64 and x86 AMD64.
>>
>> Patch 3 enables readelf/objdump to dump the SFrame fixed offsets from
>> CFA to the frame pointer (FP) and return address (RA).
>>
>> Patch 4 changes readelf/objdump to display 'f' in the SFrame
>> RA tracking column, if the architecture is using a fixed RA offset.
>> Additionally it corrects the logic to display 'u' in the SFrame
>> FP tracking column.
>>
>> Patch 5 (updated) enhances an SFrame warning message to print the human
>> readable DWARF call frame instruction name.
>>
>> Patches 6 and 7 resolve issues that cause the assembler to either
>> generate bad SFrame FDE or to silently skip it. Both issues would be
>> triggered by s390-specific SFrame error test cases introduced by the
>> separate patch series.
>>
>> Patch 8 refactors SFrame CFI opcode DW_CFA_register processing into a
>> separate function. This harmonizes the CFI opcode processing.
>>
>> Patch 9 (updated) adds verbose assembler warning messages when
>> generation of SFrame FDE is skipped.
>>
>> Patch 10 (updated) resolves an issue that causes the assembler to
>> generate bad SFrame FDE in case the FP without RA was saved on the
>> stack, which the SFrame format cannot represent. I will send two
>> alternative solution proposals as RFC.
>>
>> Patch 11 (updated) skips SFrame FDE for .cfi_window_save on all
>> architectures except AArch64, which multiplexed it with
>> .cfi_negate_ra_state.
>>
>> Patches 12 and 13 (updated) resolve issues where generation of SFrame
>> FDE was unnecessarily skipped.
>>
>> Patch 14 adds tests for the SFrame RA tracking predicate to places where
>> it was missing to align the logic.
>>
>> Patch 15 (updated) is a minor enhancement to add checks that the
>> architecture-dependent RA tracking is correctly configured.
> 
> ...
> 
>> Jens Remus (15):
>>    x86: Remove unused SFrame CFI RA register variable
>>    gas: Enhance arch-specific SFrame configuration descriptions
>>    readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>>    readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>>    gas: Print DWARF call frame insn name in SFrame warning message
>>    gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>>    gas: Warn if SFrame FDE is skipped due to non-default return column
>>    gas: Refactor SFrame CFI opcode DW_CFA_register processing
>>    gas: User readable warnings if SFrame FDE is not generated
>>    gas: Skip SFrame FDE if FP without RA on stack
>>    gas: Skip SFrame FDE if .cfi_window_save
>>    gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>>    gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>>    gas: Test predicate whether SFrame RA tracking is used
>>    gas: Validate SFrame RA tracking and fixed RA offset
>>
>>   gas/config/tc-aarch64.c                       |   6 +-
>>   gas/config/tc-aarch64.h                       |  12 +-
>>   gas/config/tc-i386.c                          |   6 +-
>>   gas/config/tc-i386.h                          |  10 +-
>>   gas/gen-sframe.c                              | 246 +++++++++++++++---
>>   gas/gen-sframe.h                              |   2 +
>>   .../gas/cfi-sframe/cfi-sframe-common-1.d      |   2 +
>>   .../gas/cfi-sframe/cfi-sframe-common-2.d      |   2 +
>>   .../gas/cfi-sframe/cfi-sframe-common-3.d      |   2 +
>>   .../gas/cfi-sframe/cfi-sframe-common-4.d      |   6 +-
>>   .../gas/cfi-sframe/cfi-sframe-common-5.d      |   6 +-
>>   .../gas/cfi-sframe/cfi-sframe-common-6.d      |   6 +-
>>   .../gas/cfi-sframe/cfi-sframe-common-7.d      |   6 +-
>>   .../gas/cfi-sframe/cfi-sframe-common-8.d      |   4 +-
>>   .../gas/cfi-sframe/cfi-sframe-x86_64-1.d      |   9 +-
>>   gas/testsuite/gas/cfi-sframe/common-empty-1.d |   4 +-
>>   gas/testsuite/gas/cfi-sframe/common-empty-2.d |   4 +-
>>   gas/testsuite/gas/cfi-sframe/common-empty-3.d |   3 +
>>   .../gas/scfi/x86_64/scfi-cfi-sections-1.d     |  11 +-
>>   .../gas/scfi/x86_64/scfi-dyn-stack-1.d        |  11 +-
>>   ld/testsuite/ld-sframe/discard.s              |   1 -
>>   ld/testsuite/ld-x86-64/sframe-plt-1.d         |   9 +-
>>   ld/testsuite/ld-x86-64/sframe-simple-1.d      |  17 +-
>>   libsframe/sframe-dump.c                       |  18 +-
>>   24 files changed, 305 insertions(+), 98 deletions(-)
> 
> Thank you for the review, Indu!
> 
> Can I go ahead and commit the series to mainline, with the review 
> feedback implemented and the offending trailers removed?

Yes.

Thanks for improving the SFrame functionality,
Indu
  

On 27.06.2024 10:28, Jens Remus wrote:
> Am 24.06.2024 um 16:23 schrieb Jens Remus:
>> Patches 1 and 2 (updated) are minor cleanups/enhancements to the
>> existing SFrame support on AArch64 and x86 AMD64.
>>
>> Patch 3 enables readelf/objdump to dump the SFrame fixed offsets from
>> CFA to the frame pointer (FP) and return address (RA).
>>
>> Patch 4 changes readelf/objdump to display 'f' in the SFrame
>> RA tracking column, if the architecture is using a fixed RA offset.
>> Additionally it corrects the logic to display 'u' in the SFrame
>> FP tracking column.
>>
>> Patch 5 (updated) enhances an SFrame warning message to print the human
>> readable DWARF call frame instruction name.
>>
>> Patches 6 and 7 resolve issues that cause the assembler to either
>> generate bad SFrame FDE or to silently skip it. Both issues would be
>> triggered by s390-specific SFrame error test cases introduced by the
>> separate patch series.
>>
>> Patch 8 refactors SFrame CFI opcode DW_CFA_register processing into a
>> separate function. This harmonizes the CFI opcode processing.
>>
>> Patch 9 (updated) adds verbose assembler warning messages when
>> generation of SFrame FDE is skipped.
>>
>> Patch 10 (updated) resolves an issue that causes the assembler to
>> generate bad SFrame FDE in case the FP without RA was saved on the
>> stack, which the SFrame format cannot represent. I will send two
>> alternative solution proposals as RFC.
>>
>> Patch 11 (updated) skips SFrame FDE for .cfi_window_save on all
>> architectures except AArch64, which multiplexed it with
>> .cfi_negate_ra_state.
>>
>> Patches 12 and 13 (updated) resolve issues where generation of SFrame
>> FDE was unnecessarily skipped.
>>
>> Patch 14 adds tests for the SFrame RA tracking predicate to places where
>> it was missing to align the logic.
>>
>> Patch 15 (updated) is a minor enhancement to add checks that the
>> architecture-dependent RA tracking is correctly configured.
> 
> ...
> 
>> Jens Remus (15):
>>    x86: Remove unused SFrame CFI RA register variable
>>    gas: Enhance arch-specific SFrame configuration descriptions
>>    readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>>    readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>>    gas: Print DWARF call frame insn name in SFrame warning message
>>    gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>>    gas: Warn if SFrame FDE is skipped due to non-default return column
>>    gas: Refactor SFrame CFI opcode DW_CFA_register processing
>>    gas: User readable warnings if SFrame FDE is not generated
>>    gas: Skip SFrame FDE if FP without RA on stack
>>    gas: Skip SFrame FDE if .cfi_window_save
>>    gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>>    gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>>    gas: Test predicate whether SFrame RA tracking is used
>>    gas: Validate SFrame RA tracking and fixed RA offset
>>
>>   gas/config/tc-aarch64.c                       |   6 +-
>>   gas/config/tc-aarch64.h                       |  12 +-
>>   gas/config/tc-i386.c                          |   6 +-
>>   gas/config/tc-i386.h                          |  10 +-
>>   gas/gen-sframe.c                              | 246 +++++++++++++++---
>>   gas/gen-sframe.h                              |   2 +
>>   .../gas/cfi-sframe/cfi-sframe-common-1.d      |   2 +
>>   .../gas/cfi-sframe/cfi-sframe-common-2.d      |   2 +
>>   .../gas/cfi-sframe/cfi-sframe-common-3.d      |   2 +
>>   .../gas/cfi-sframe/cfi-sframe-common-4.d      |   6 +-
>>   .../gas/cfi-sframe/cfi-sframe-common-5.d      |   6 +-
>>   .../gas/cfi-sframe/cfi-sframe-common-6.d      |   6 +-
>>   .../gas/cfi-sframe/cfi-sframe-common-7.d      |   6 +-
>>   .../gas/cfi-sframe/cfi-sframe-common-8.d      |   4 +-
>>   .../gas/cfi-sframe/cfi-sframe-x86_64-1.d      |   9 +-
>>   gas/testsuite/gas/cfi-sframe/common-empty-1.d |   4 +-
>>   gas/testsuite/gas/cfi-sframe/common-empty-2.d |   4 +-
>>   gas/testsuite/gas/cfi-sframe/common-empty-3.d |   3 +
>>   .../gas/scfi/x86_64/scfi-cfi-sections-1.d     |  11 +-
>>   .../gas/scfi/x86_64/scfi-dyn-stack-1.d        |  11 +-
>>   ld/testsuite/ld-sframe/discard.s              |   1 -
>>   ld/testsuite/ld-x86-64/sframe-plt-1.d         |   9 +-
>>   ld/testsuite/ld-x86-64/sframe-simple-1.d      |  17 +-
>>   libsframe/sframe-dump.c                       |  18 +-
>>   24 files changed, 305 insertions(+), 98 deletions(-)
> 
> Thank you for the review, Indu!
> 
> Can I go ahead and commit the series to mainline, with the review 
> feedback implemented and the offending trailers removed?

I haven't been following closely what specifically Indu asked for. In
general, if he's happy with the sframe-specific changes, respective
changes can have my (implicit) okay. Target-specific changes will want
a target-specific okay, though. From the titles I can only identify
patch 1 as having x86-specific aspects. Looks like it's only patch 2
which has further arch-specific changes - I'm okay with the x86 parts,
but I can't speak for Arm64 (well, technically I could, but I'd like
to avoid doing so unless strictly necessary).

Jan
  

Am 27.06.2024 um 10:39 schrieb Jan Beulich:
> On 27.06.2024 10:28, Jens Remus wrote:
>> Am 24.06.2024 um 16:23 schrieb Jens Remus:
>>> Patches 1 and 2 (updated) are minor cleanups/enhancements to the
>>> existing SFrame support on AArch64 and x86 AMD64.
...
>>> Jens Remus (15):
>>>     x86: Remove unused SFrame CFI RA register variable
>>>     gas: Enhance arch-specific SFrame configuration descriptions
>>>     readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>>>     readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>>>     gas: Print DWARF call frame insn name in SFrame warning message
>>>     gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>>>     gas: Warn if SFrame FDE is skipped due to non-default return column
>>>     gas: Refactor SFrame CFI opcode DW_CFA_register processing
>>>     gas: User readable warnings if SFrame FDE is not generated
>>>     gas: Skip SFrame FDE if FP without RA on stack
>>>     gas: Skip SFrame FDE if .cfi_window_save
>>>     gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>>>     gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>>>     gas: Test predicate whether SFrame RA tracking is used
>>>     gas: Validate SFrame RA tracking and fixed RA offset
>>>
>>>    gas/config/tc-aarch64.c                       |   6 +-
>>>    gas/config/tc-aarch64.h                       |  12 +-
>>>    gas/config/tc-i386.c                          |   6 +-
>>>    gas/config/tc-i386.h                          |  10 +-
>>>    gas/gen-sframe.c                              | 246 +++++++++++++++---
>>>    gas/gen-sframe.h                              |   2 +
>>>    .../gas/cfi-sframe/cfi-sframe-common-1.d      |   2 +
>>>    .../gas/cfi-sframe/cfi-sframe-common-2.d      |   2 +
>>>    .../gas/cfi-sframe/cfi-sframe-common-3.d      |   2 +
>>>    .../gas/cfi-sframe/cfi-sframe-common-4.d      |   6 +-
>>>    .../gas/cfi-sframe/cfi-sframe-common-5.d      |   6 +-
>>>    .../gas/cfi-sframe/cfi-sframe-common-6.d      |   6 +-
>>>    .../gas/cfi-sframe/cfi-sframe-common-7.d      |   6 +-
>>>    .../gas/cfi-sframe/cfi-sframe-common-8.d      |   4 +-
>>>    .../gas/cfi-sframe/cfi-sframe-x86_64-1.d      |   9 +-
>>>    gas/testsuite/gas/cfi-sframe/common-empty-1.d |   4 +-
>>>    gas/testsuite/gas/cfi-sframe/common-empty-2.d |   4 +-
>>>    gas/testsuite/gas/cfi-sframe/common-empty-3.d |   3 +
>>>    .../gas/scfi/x86_64/scfi-cfi-sections-1.d     |  11 +-
>>>    .../gas/scfi/x86_64/scfi-dyn-stack-1.d        |  11 +-
>>>    ld/testsuite/ld-sframe/discard.s              |   1 -
>>>    ld/testsuite/ld-x86-64/sframe-plt-1.d         |   9 +-
>>>    ld/testsuite/ld-x86-64/sframe-simple-1.d      |  17 +-
>>>    libsframe/sframe-dump.c                       |  18 +-
>>>    24 files changed, 305 insertions(+), 98 deletions(-)
>>
>> Thank you for the review, Indu!
>>
>> Can I go ahead and commit the series to mainline, with the review
>> feedback implemented and the offending trailers removed?
> 
> I haven't been following closely what specifically Indu asked for. In
> general, if he's happy with the sframe-specific changes, respective
> changes can have my (implicit) okay. Target-specific changes will want
> a target-specific okay, though. From the titles I can only identify
> patch 1 as having x86-specific aspects. Looks like it's only patch 2
> which has further arch-specific changes - I'm okay with the x86 parts,
> but I can't speak for Arm64 (well, technically I could, but I'd like
> to avoid doing so unless strictly necessary).

Richard and Marcus, could one of you please have a short look at patch 2 
of this series, as it contains changes to the AArch64-specific assembler 
code? Note that it only rewords and harmonizes generic comments that are 
SFrame specific. Therefore I had not considered to Cc you as AArch64 
maintainers nor the x86-64 maintainers and assumed Indu's ok as SFrame 
maintainer would be sufficient.

Thanks and regards,
Jens
  

On 27/06/2024 12:02, Jens Remus wrote:
> Am 27.06.2024 um 10:39 schrieb Jan Beulich:
>> On 27.06.2024 10:28, Jens Remus wrote:
>>> Am 24.06.2024 um 16:23 schrieb Jens Remus:
>>>> Patches 1 and 2 (updated) are minor cleanups/enhancements to the
>>>> existing SFrame support on AArch64 and x86 AMD64.
> ...
>>>> Jens Remus (15):
>>>>     x86: Remove unused SFrame CFI RA register variable
>>>>     gas: Enhance arch-specific SFrame configuration descriptions
>>>>     readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>>>>     readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>>>>     gas: Print DWARF call frame insn name in SFrame warning message
>>>>     gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>>>>     gas: Warn if SFrame FDE is skipped due to non-default return column
>>>>     gas: Refactor SFrame CFI opcode DW_CFA_register processing
>>>>     gas: User readable warnings if SFrame FDE is not generated
>>>>     gas: Skip SFrame FDE if FP without RA on stack
>>>>     gas: Skip SFrame FDE if .cfi_window_save
>>>>     gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>>>>     gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>>>>     gas: Test predicate whether SFrame RA tracking is used
>>>>     gas: Validate SFrame RA tracking and fixed RA offset
>>>>
>>>>    gas/config/tc-aarch64.c                       |   6 +-
>>>>    gas/config/tc-aarch64.h                       |  12 +-
>>>>    gas/config/tc-i386.c                          |   6 +-
>>>>    gas/config/tc-i386.h                          |  10 +-
>>>>    gas/gen-sframe.c                              | 246 +++++++++++++++---
>>>>    gas/gen-sframe.h                              |   2 +
>>>>    .../gas/cfi-sframe/cfi-sframe-common-1.d      |   2 +
>>>>    .../gas/cfi-sframe/cfi-sframe-common-2.d      |   2 +
>>>>    .../gas/cfi-sframe/cfi-sframe-common-3.d      |   2 +
>>>>    .../gas/cfi-sframe/cfi-sframe-common-4.d      |   6 +-
>>>>    .../gas/cfi-sframe/cfi-sframe-common-5.d      |   6 +-
>>>>    .../gas/cfi-sframe/cfi-sframe-common-6.d      |   6 +-
>>>>    .../gas/cfi-sframe/cfi-sframe-common-7.d      |   6 +-
>>>>    .../gas/cfi-sframe/cfi-sframe-common-8.d      |   4 +-
>>>>    .../gas/cfi-sframe/cfi-sframe-x86_64-1.d      |   9 +-
>>>>    gas/testsuite/gas/cfi-sframe/common-empty-1.d |   4 +-
>>>>    gas/testsuite/gas/cfi-sframe/common-empty-2.d |   4 +-
>>>>    gas/testsuite/gas/cfi-sframe/common-empty-3.d |   3 +
>>>>    .../gas/scfi/x86_64/scfi-cfi-sections-1.d     |  11 +-
>>>>    .../gas/scfi/x86_64/scfi-dyn-stack-1.d        |  11 +-
>>>>    ld/testsuite/ld-sframe/discard.s              |   1 -
>>>>    ld/testsuite/ld-x86-64/sframe-plt-1.d         |   9 +-
>>>>    ld/testsuite/ld-x86-64/sframe-simple-1.d      |  17 +-
>>>>    libsframe/sframe-dump.c                       |  18 +-
>>>>    24 files changed, 305 insertions(+), 98 deletions(-)
>>>
>>> Thank you for the review, Indu!
>>>
>>> Can I go ahead and commit the series to mainline, with the review
>>> feedback implemented and the offending trailers removed?
>>
>> I haven't been following closely what specifically Indu asked for. In
>> general, if he's happy with the sframe-specific changes, respective
>> changes can have my (implicit) okay. Target-specific changes will want
>> a target-specific okay, though. From the titles I can only identify
>> patch 1 as having x86-specific aspects. Looks like it's only patch 2
>> which has further arch-specific changes - I'm okay with the x86 parts,
>> but I can't speak for Arm64 (well, technically I could, but I'd like
>> to avoid doing so unless strictly necessary).
> 
> Richard and Marcus, could one of you please have a short look at patch 2 of this series, as it contains changes to the AArch64-specific assembler code? Note that it only rewords and harmonizes generic comments that are SFrame specific. Therefore I had not considered to Cc you as AArch64 maintainers nor the x86-64 maintainers and assumed Indu's ok as SFrame maintainer would be sufficient.

The aarch64 changes look to me to all be comment clarifications related to the SFrame stuff.  If Indu is happy, then so am I.

R.

> 
> Thanks and regards,
> Jens
  

Am 27.06.2024 um 10:32 schrieb Indu Bhagat:
> On 6/27/24 01:28, Jens Remus wrote:
>> Am 24.06.2024 um 16:23 schrieb Jens Remus:
...
>>> Jens Remus (15):
>>>    x86: Remove unused SFrame CFI RA register variable
>>>    gas: Enhance arch-specific SFrame configuration descriptions
>>>    readelf/objdump: Dump SFrame CFA fixed FP and RA offsets
>>>    readelf/objdump: Display SFrame fixed RA offset as 'f' in dump
>>>    gas: Print DWARF call frame insn name in SFrame warning message
>>>    gas: Skip SFrame FDE if CFI specifies non-FP/SP base register
>>>    gas: Warn if SFrame FDE is skipped due to non-default return column
>>>    gas: Refactor SFrame CFI opcode DW_CFA_register processing
>>>    gas: User readable warnings if SFrame FDE is not generated
>>>    gas: Skip SFrame FDE if FP without RA on stack
>>>    gas: Skip SFrame FDE if .cfi_window_save
>>>    gas: Don't skip SFrame FDE if .cfi_register specifies RA w/o tracking
>>>    gas: Don't skip SFrame FDE if .cfi_register specifies SP register
>>>    gas: Test predicate whether SFrame RA tracking is used
>>>    gas: Validate SFrame RA tracking and fixed RA offset
...
>> Can I go ahead and commit the series to mainline, with the review 
>> feedback implemented and the offending trailers removed?
> 
> Yes.

Committed to mainline with approval from Indu, Jan, and Richard. Thank 
you for the review!

Regards,
Jens