Skip to content

[lld][test] Precommit test for ld -r links with FatLTO PIC objects #92817

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 31, 2024
Merged

[lld][test] Precommit test for ld -r links with FatLTO PIC objects #92817

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7ae4266
[𝘀𝗽𝗿] initial version
ilovepi May 20, 2024
c09de94
Fix filename in RUN line
ilovepi May 20, 2024
44140c1
Reuse existing objects in test
ilovepi May 21, 2024
f02bd6e
Set section type to SHT_LLVM_LTO
ilovepi May 21, 2024
cded743
Update test to use llvm-readelf
ilovepi May 23, 2024
8318fad
CD into temp dir
ilovepi May 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions lld/test/ELF/fatlto/fatlto.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,23 @@
; RUN: rm -rf %t && split-file %s %t

;; Ensure that input files contain .llvm.lto section.
; RUN: llc %t/a-LTO.ll --filetype=obj -o %t/a-fatLTO.o
; RUN: llc %t/a-LTO.ll --filetype=obj -o %t/a-fatLTO.o --relocation-model=pic
; RUN: opt < %t/a-LTO.ll --module-summary -o %t/a-fatLTO.bc
; RUN: llvm-objcopy --add-section=.llvm.lto=%t/a-fatLTO.bc --set-section-flags=.llvm.lto=exclude %t/a-fatLTO.o
; RUN: llvm-objcopy --add-section=.llvm.lto=%t/a-fatLTO.bc --set-section-flags=.llvm.lto=exclude --set-section-type=.llvm.lto=0x6fff4c0c %t/a-fatLTO.o
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The number of %t occurrences is large now.

Perhaps we should switch to ; RUN: rm -rf %t && split-file %s %t && cd %t and remove these %t/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that you prefer the cd approach, but I find it hard to re-run commands that require a cd into a temp directory when iterating on tests. Leaving %t/ in the paths allow me to easily copy the failing command and run it from the build directory. If you feel strongly, I can still change it, but my preference is to leave this as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MaskRay , just double checking that you're OK with this as is before I land this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test is complex enough that removing some %t/ occurrences will help readability...

I don't use llvm-lit for one-shot test invocation. I use ff fatlto.test and it's quite straightforward to copy & paste one single command...

#!/usr/bin/env python3
# Simplified lit runner for a single test.
import argparse
import os, re, sys
from ipdb import set_trace as bp

def main():
    ap = argparse.ArgumentParser()
    ap.add_argument('-b', '--build', default='/tmp/Debug', help='build directory')
    ap.add_argument('-d', '--debug', type=int, default=-1, help='rr record the specified command')
    ap.add_argument('-r', '--run', type=int, default=-1, help='run the specified command')
    ap.add_argument('--release', action='store_true', help='use /tmp/Rel as the build directory')
    ap.add_argument('--binutils', action='store_true')
    ap.add_argument('-n', action='store_true', help='dry run')
    ap.add_argument('file')
    args = ap.parse_args()
    if args.debug >= 0:
        args.run = args.debug

    home = os.path.expanduser('~')
    build_dir = '/tmp/Rel' if args.release else args.build
    os.environ["PATH"] = os.path.expanduser(build_dir + '/bin') + os.pathsep + os.environ['PATH']
    os.environ['LLVM_DISABLE_SYMBOLIZATION'] = '1'
    os.environ['LLD_IN_TEST'] = '1'
    os.environ['LLD_VERSION'] = 'LLD 1.0'

    # GNU binutils
    if args.binutils:
        del sys.argv[1]
        os.environ["PATH"] = os.path.expanduser('~/Dev/binutils-gdb/out/release/ld') + os.pathsep + os.environ['PATH']

    cwd = os.getcwd()
    fname = os.path.realpath(args.file)
    tmp = os.path.join(os.path.dirname(fname), 'a')
    dirname = os.path.dirname(fname).replace(home, '$HOME')
    lineno = 0
    last = ''
    try:
       fh = open(fname)
    except FileNotFoundError:
        print('not exist', file=sys.stderr)
        return
    substs = sorted([
        ('/p', lambda: os.path.relpath(dirname, cwd)),
        ('/t', lambda: os.path.relpath(tmp, cwd)),
        # See llvm/utils/lit/lit/llvm/config.py
        ('clang', lambda: 'clang'),
        ('clang_cc1', lambda: f'clang -cc1 -internal-isystem {build_dir}/lib/clang/17/include -nostdsysteminc'),
        ('errc_EACCES', lambda: '"Permission denied"'),
        ('itanium_abi_triple', lambda: 'x86_64'),
        ('llc_dwarf', lambda: 'llc'),
        # See lld/test/MachO/lit.local.cfg
        ('lld', lambda: os.path.expanduser('ld64.lld -arch x86_64 -platform_version macos 11.0 11.0 -syslibroot ~/llvm/lld/test/MachO/Inputs/MacOSX.sdk -lSystem -fatal_warnings')),
        ('no-fatal-warnings-lld', lambda: os.path.expanduser('ld64.lld -arch x86_64 -platform_version macos 11.0 11.0 -syslibroot ~/llvm/lld/test/MachO/Inputs/MacOSX.sdk -lSystem')),
        ('p', lambda: os.path.relpath(dirname, cwd)),
        ('python', lambda: 'python3'),
        ('run', lambda: ''),
        ('s', lambda: os.path.relpath(fname, cwd)),
        ('S', lambda: os.path.relpath(dirname, cwd)),
        ('t', lambda: os.path.relpath(tmp, cwd)),
    ], key=lambda x: -len(x[0]))

    with fh as f:
        for line in f.readlines():
            line = line.strip()
            lineno += 1
            if 'END.' in line: break
            match = re.search(r'RUN:[ \t]', line)
            if not match:
                last = ''
                continue
            line = line[match.end():]
            if line[-1] == '\\':
                last += line[:-1]
                continue
            line = last + line
            last = ''

            if 'rm ' in line and ('..' in line or (not re.search(r'rm (-f|-fr|-rf) %t', line))):
                continue

            line1 = ''
            idx = 0
            while (idx1 := line.find('%', idx)) != -1:
                line1 += line[idx:idx1]
                idx = idx1+1
                for s in substs:
                    if line[idx:].startswith(s[0]):
                        line1 += s[1]()
                        idx += len(s[0])
                        break
                else:
                    line1 += line[idx-1]
            line = line1 + line[idx:]

            if args.run < 0:
                print(f'{lineno:^2} {line}')
            if not args.n:
                if os.system(line) != 0:
                    sys.exit(1)
            if args.run == lineno:
                if (idx := line.find('|')) >= 0:
                    line = line[:idx]
                line = f'{build_dir}/bin/{line}'
                if args.debug >= 0:
                    line = 'rr record ' + line
                print(f'### {line}')
                if not args.n:
                    if os.system(line) != 0:
                        sys.exit(1)
                    if args.debug >= 0:
                        os.execvp('rr', ('rr', 'replay', '-d', 'cgdb'))
                break

            if match := re.search(r'\bcd ([/.\w]+)', line):
                os.chdir(match.group(1))
                cwd = os.getcwd()

main()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, while I'm going to change the test to your preferred format, LIT_FILTER= has been a great way to run some subset of tests, and when it fails, I can examine the intermediate outputs by just copying the printed RUN: ... command. In the case where there is a directory change at the top of the test, I can't just grab that command and run it without either figuring out which directory to CD into or prepending the correct prefix to every path in the command. To me the directory change is an anti-pattern that blocks a workflow used extensivly by other developers. It was even mentioned recently on discourse when the output from LIT was modified, https://discourse.llvm.org/t/rfc-improving-lits-debug-output/72839.

For me getting something I can copy, like:

 /path/to/opt -some-pass -o - < /path/to/test/file | FileCheck /path/to/test/file

is really convenient. While the script you provided seems useful, I'd rather not have to use a script just to run a single shell command that I often want to edit by adding different flags or capturing its output.


; RUN: llc %t/main-LTO.ll --filetype=obj -o %t/main-fatLTO.o

; RUN: llc %t/main-LTO.ll --filetype=obj -o %t/main-fatLTO.o --relocation-model=pic
; RUN: opt < %t/main-LTO.ll --module-summary -o %t/main-fatLTO.bc
; RUN: llvm-objcopy --add-section=.llvm.lto=%t/main-fatLTO.bc --set-section-flags=.llvm.lto=exclude %t/main-fatLTO.o
; RUN: llvm-objcopy --add-section=.llvm.lto=%t/main-fatLTO.bc --set-section-flags=.llvm.lto=exclude --set-section-type=.llvm.lto=0x6fff4c0c %t/main-fatLTO.o

; RUN: llvm-readobj -S %t/a-fatLTO.o | FileCheck --check-prefix=HAS_LLVM_LTO %s
; RUN: llvm-readobj -S %t/main-fatLTO.o | FileCheck --check-prefix=HAS_LLVM_LTO %s

;; Make sure that the section flags are set correctly
; HAS_LLVM_LTO: Name: .llvm.lto
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, newer tests probably should consider llvm-readelf -S instead llvm-readobj. The tabular output is easy to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems to work fine, so I've updated the checks in this file.

; HAS_LLVM_LTO-NEXT: Type: SHT_LLVM_LTO
; HAS_LLVM_LTO-NEXT: Flags
; HAS_LLVM_LTO-NEXT: SHF_EXCLUDE

;; Final executable should not have .llvm.lto section no matter what the target is.
; RUN: ld.lld -o %t/foo-fatLTO %t/a-fatLTO.o %t/main-fatLTO.o --fat-lto-objects
Expand Down Expand Up @@ -49,6 +59,17 @@
; RUN: ld.lld -o %t/foo-fatLTO.archive %t/a.a %t/main-LTO.bc --fat-lto-objects
; RUN: cmp %t/foo-fatLTO.archive %t/foo-LTO

;; Test FatLTO works with relocatable links using PIC objects
;; Currently, with PIC relocatable links, FatLTO sections are treated as
;; orphan sections and incorrectly concatenated together. This test verifies
;; the current behavior, but should be fixed to either merge those sections
;; correctly, or to drop them altogether.
; RUN: llvm-ar rcs %t/fatLTO-pic.a %t/a-fatLTO.o %t/main-fatLTO.o
; RUN: llvm-readobj -S %t/fatLTO-pic.a | FileCheck --check-prefix=HAS_LLVM_LTO %s

; RUN: ld.lld --whole-archive %t/fatLTO-pic.a -r -o %t/fatLTO-pic-reolcatable.o
; RUN: llvm-readobj -S %t/fatLTO-pic-reolcatable.o | FileCheck --check-prefix=HAS_LLVM_LTO %s

;--- a-LTO.ll
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
Expand Down
Loading