Skip to content

Conversation

@am11
Copy link
Member

@am11 am11 commented Jul 6, 2022

See #71270 (comment)

Up until recently (#71446), we were using binutils' objcopy on Linux. We should fallback to that if llvm-objcopy provided by selected (older) toolchain does not meet the requirement.

@ghost
Copy link

ghost commented Jul 6, 2022

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 6, 2022
@am11 am11 requested review from hoyosjs, janvorli and jkotas July 6, 2022 23:20
@ghost
Copy link

ghost commented Jul 6, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

See #71270 (comment)

Up until recently (#71446), we were using binutils' objcopy on Linux. We should fallback to that if llvm-objcopy provided by selected (older) toolchain does not meet the requirement.

Author: am11
Assignees: -
Labels:

area-Infrastructure-coreclr, community-contribution

Milestone: -

OUTPUT_VARIABLE OBJCOPY_HELP_OUTPUT
)

if(CLR_CMAKE_TARGET_ANDROID)
Copy link
Member Author

Choose a reason for hiding this comment

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

This block was unused.

@am11
Copy link
Member Author

am11 commented Jul 6, 2022

@jkotas, I have not changed anything for NativeAOT, since there we anyway require recent versions of llvm / clang (and only use objcopy when user explicitly sets -p:StripSymbols=true).

endif()
# if llvm-objcopy does not support --only-keep-debug argument, try to locate binutils' objcopy
if (CMAKE_C_COMPILER_ID MATCHES "Clang" AND NOT "${OBJCOPY_HELP_OUTPUT}" MATCHES "--only-keep-debug")
set(TOOLSET_PREFIX "")
Copy link
Member

Choose a reason for hiding this comment

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

llvm also has a only-keep-debug flag, only with one dash though. Any reason to favor binutils over llvm's and just adjusting the flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

My test was:

$ docker run -it ubuntu:18.04

# inside the container, install different versions
$ apt update; apt install -y llvm-6 llvm-7 llvm-8 llvm-9 llvm-10

# test for only-keep-debug

# v6 - desired argument is unsupported
$ llvm-objcopy-6.0 --help | grep only-keep-debug         
# nothing found

# v7 - supported with single hyphen but is a no-op
$ llvm-objcopy-7 --help | grep only-keep-debug
  -only-keep-debug        Currently ignored. Only for compaitability with GNU objcopy.

# v8 - same as v7
$ llvm-objcopy-8 --help | grep only-keep-debug
  -only-keep-debug        Currently ignored. Only for compatibility with GNU objcopy.

# v9 - supported with double hyphen
$ llvm-objcopy-9 --help | grep only-keep-debug
  --only-keep-debug       Clear sections that would not be stripped by --strip-debug. Currently only implemented for COFF.

# v10 - same as v9
$ llvm-objcopy-10 --help | grep only-keep-debug
  --only-keep-debug       Produce a debug file as the output that only preserves contents of sections useful for debugging purposes

We want only-keep-debug for consistent output; avoid binary size bloat and avoid repeating issues like #49081. Therefore, we fallback to binutils' objcopy when llvm-objcopy is incapable of stripping the symbols. As you can see above, versions of llvm-objcopy which support double hyphened argument provide non-nop functionality.

Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

Then this is the right fix. Thanks!

@hoyosjs
Copy link
Member

hoyosjs commented Jul 7, 2022

Failure is #70450

@hoyosjs hoyosjs merged commit 34383d1 into dotnet:main Jul 7, 2022
This was referenced Jul 15, 2022
akoeplinger pushed a commit that referenced this pull request Jul 15, 2022
Unblocks #71725.
Followup: #71742

Prior to #71742, we were calling `locate_toolchain_exec` once per tool. Calling it twice with different prefix gives us the old result, which is something we don't want (we only call this local function a few times in this file, and each time for intention to "lookup" the executable).
@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Infrastructure-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants