The current relaxation implementation is not correctly adjusting the
size and offsets of fragements in one section based on changes in size
of another if the layout order of the two happened to be such that the
former was visited before the later. Therefore, we need to invalidate
the fragments in all sections after each iteration of relaxation, and
possibly further relax some of them in the next ieration. This fixes
PR#45190.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
| Time | Test | |
|---|---|---|
| 210 ms | libunwind.libunwind::Unknown Unit Message (" ") Command: ['/usr/bin/clang++', '-o', '/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libunwind/test/Output/frameheadercache_test.pass.cpp.o', '-x', 'c++', '/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/test/frameheadercache_test.pass.cpp', '-c', '-v', '-ftemplate-depth=270', '-Werror=thread-safety', '-DLIBUNWIND_NO_TIMER', '-funwind-tables', '-std=c++2a', '-I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/include', '-D__STDC_FORMAT_MACROS', '-D__STDC_LIMIT_MACROS', '-D__STDC_CONSTANT_MACROS', '-I/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/../libcxx/test/support', '-DLIBCXX_FILESYSTEM_STATIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libcxx/test/std/input.output/filesystems/Inputs/static_test_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_ROOT="/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/projects/libunwind/test/filesystem/Output/dynamic_env"', '-DLIBCXX_FILESYSTEM_DYNAMIC_TEST_HELPER="/usr/bin/python /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/libunwind/../libcxx/test/support/filesystem_dynamic_test_helper.py"', '-D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER', '-Wall', '-Wextra', '-Werror', '-Wuser-defined-warnings', '-Wshadow', '-Wno-unused-command-line-argument', '-Wno-attributes', '-Wno-pessimizing-move', '-Wno-c++11-extensions', '-Wno-user-defined-literals', '-Wno-noexcept-type', '-Wsign-compare', '-Wunused-variable', '-Wunused-parameter', '-Wunreachable-code', '-c']
Exit Code: 1
Standard Error:
|
Event Timeline
| llvm/test/MC/X86/relax-offset.s | ||
|---|---|---|
| 16 | NOT pattern should be carefully used. Without a positive check with the same diagnostic, a negative check can easily get stale and don't check anything. For this case (a regression test), an error will cause llvm-mc to return 1 instead of 0, llvm-mc -filetype=obj -triple=i386 %s -o /dev/null is sufficient. | |
| llvm/test/MC/X86/relax-offset.s | ||
|---|---|---|
| 16 |
Just to second this, and add my negative experiences with CHECK-NOT; make sure the test is red before your patch is applied, then green after it's applied. Test Driven Development. I too have seen CHECK-NOT's not actually test anything. Also, @MaskRay , this check is for an assertion failure. Is it common to test for asserts, which may not be enabled when running the tests in release mode? | |
Comment Actions
Reduced test case:
.section .entry.text, "ax"
.skip 144f-143f,0x0
.pushsection .altinstr_replacement,"ax"
143:
jmp .Lint80_keep_stack
144:
.popsection
.Lint80_keep_stack:
This does not appear to relate to the recent alignment padding work. Test continues to fail with -x86-pad-for-align=0 -x86-pad-for-branch-align=0
Comment Actions
The proposed fix is insufficient. Consider the case where a change in size of the skip requires further relaxation in that section or another. This needs to cause another iteration of relaxation.
Comment Actions
Agreed. In that case we can probably check whether the offset of any valid fragment is changed, and if so trigger another pass on the layout. Although I am not sure on (1) how to come up with a test case, and (2) if that will eventually converge.
Comment Actions
This does not appear to relate to the recent alignment padding work. Test continues to fail with -x86-pad-for-align=0 -x86-pad-for-branch-align=0
I am not sure I followed here but I ran the test case as follows and it still passed.
llvm-mc -filetype=obj -triple=i386 relax-offset.s -o /dev/null -x86-pad-for-align=0 -x86-pad-for-branch-align=0
Comment Actions
Er, I assume you mean "passed on the build with your change" correct?
I specifically meant "continued to fail on top of tree with those padding flags disabled (set to 0)".
Comment Actions
LGTM, though please make sure you update the description when landing! The description is incomplete, and doesn't cover the iterative part.
| llvm/test/MC/X86/relax-offset.s | ||
|---|---|---|
| 2 | llvm-objdump --headers -> llvm-readelf -S llvm-objdump --headers does not match GNU and can cause confusion. | |
Comment Actions
Do we know which commit caused the regression? Is it related to one of the recent x86 instruction padding patches?
| llvm/test/MC/X86/relax-offset.s | ||
|---|---|---|
| 7 | .section .text1,"ax" is slightly better, otherwise objdump -d does not disassemble. | |
Comment Actions
It's not a regression. The current relaxation implementation has never considered (or intended to support) this corner case.
Comment Actions
The main reason for the newly added test case fail is that MCFragment caches its offset (for faster layout), but does not cache its size. So when the size of a fragment changes, the offset of the fragment behind it will change. So if the subsequent fragment does not update its cached offset (because it is considered as valid), some checks will fail.