Page MenuHomePhabricator

[MC] Recalculate fragment offsets after relaxation
ClosedPublic

Authored by jcai19 on Mar 12 2020, 6:21 PM.

Details

Summary

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.

Diff Detail

Unit TestsFailed

TimeTest
240 mslibunwind.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

jcai19 created this revision.Mar 12 2020, 6:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2020, 6:21 PM
jcai19 updated this revision to Diff 250118.Mar 12 2020, 6:48 PM

Attach bug id.

jcai19 updated this revision to Diff 250122.Mar 12 2020, 9:23 PM

Update the patch.

MaskRay added inline comments.Mar 12 2020, 10:06 PM
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

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.

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?

jcai19 updated this revision to Diff 250274.Mar 13 2020, 12:03 PM

Update tests based on comments.

jcai19 marked an inline comment as done.Mar 13 2020, 12:04 PM
jcai19 removed a subscriber: MaskRay.

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

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.

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.

jcai19 updated this revision to Diff 250316.Mar 13 2020, 3:35 PM

Update test case.

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

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

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)".

jcai19 updated this revision to Diff 250329.Mar 13 2020, 5:37 PM

Update based on comments from https://reviews.llvm.org/D76166.

reames accepted this revision.Mar 16 2020, 3:26 PM

LGTM, though please make sure you update the description when landing! The description is incomplete, and doesn't cover the iterative part.

This revision is now accepted and ready to land.Mar 16 2020, 3:26 PM
jcai19 updated this revision to Diff 250653.Mar 16 2020, 4:44 PM

Update commit message.

jcai19 edited the summary of this revision. (Show Details)Mar 16 2020, 4:45 PM
MaskRay added inline comments.Mar 16 2020, 6:31 PM
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.

skan accepted this revision.Mar 16 2020, 6:32 PM

LGTM

Do we know which commit caused the regression? Is it related to one of the recent x86 instruction padding patches?

MaskRay added inline comments.Mar 16 2020, 6:38 PM
llvm/test/MC/X86/relax-offset.s
7

.section .text1,"ax" is slightly better, otherwise objdump -d does not disassemble.

skan added a comment.Mar 16 2020, 7:03 PM

Do we know which commit caused the regression? Is it related to one of the recent x86 instruction padding patches?

It's not a regression. The current relaxation implementation has never considered (or intended to support) this corner case.

skan added a comment.Mar 16 2020, 7:44 PM

Do we know which commit caused the regression? Is it related to one of the recent x86 instruction padding patches?

It's not a regression. The current relaxation implementation has never considered (or intended to support) this corner case.

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.

This revision was automatically updated to reflect the committed changes.