Skip to content
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.

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

makefiles/toolchain: fallback to 'objdump' #11547

Merged
merged 1 commit into from
May 28, 2019

Conversation

cladmi
Copy link
Contributor

@cladmi cladmi commented May 20, 2019

Contribution description

When '$(PREFIX)objdump' is not present fallback to 'objdump'.
'objdump' is used when flashing for some boards but the toolchain may
not be installed when building in docker.

If none is found, I fallback to saying 'false' but not sure if it is a
decent solution or not. Maybe just always default to 'objdump' ?

This will allow using 'objdump' in 'cpu/kinetis/dist/check-fcfield-elf.sh'.

Question

Should the default be false or objdump which may not be present ?

Testing procedure

BOARD=samr21-xpro make --no-print-directory -C examples/hello-world/ info-debug-variable-OBJDUMP
/opt/gcc-arm-none-eabi-7-2018-q2-update/bin/arm-none-eabi-objdump
PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" BOARD=samr21-xpro make --no-print-directory -C examples/hello-world/ info-debug-variable-OBJDUMP
/bin/sh: 1: arm-none-eabi-gcc: not found
/usr/bin/objdump

Removed in the last version

And another one, that I do not ask to test but review the result more

sudo mv /usr/bin/objdump /usr/bin/objdump.save

PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" BOARD=samr21-xpro make --no-print-directory -C examples/hello-world/ info-debug-variable-OBJDUMP
/bin/sh: 1: arm-none-eabi-gcc: not found
false

sudo mv /usr/bin/objdump.save /usr/bin/objdump

It also solves the main issue to use objdump when flashing kinetis boards when arm-none-eabi-ojbdump is not present. See main PR #11545

Issues/PRs references

Part of #11545 to flash kinetis without arm toolchain.
Which is part of #10870 to only use toolchain from docker.

@cladmi cladmi added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system labels May 20, 2019
@cladmi cladmi added this to the Release 2019.07 milestone May 20, 2019
Copy link
Member

@smlng smlng left a comment

Choose a reason for hiding this comment

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

Tested ACK, works on macOS.

I prefer to first try plain (non arch specific) objdump and if that is not present fail.

@smlng smlng added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels May 23, 2019
@smlng
Copy link
Member

smlng commented May 23, 2019

@cladmi if you think this is good to go, feel free to hit the green button.

@cladmi
Copy link
Contributor Author

cladmi commented May 23, 2019

@smlng you mean failing with objdump or with false then ?

If objdump is not found you would get a shell error command not found on usage, with false you would get a 1 exit code from the function.

Currently you get a arm-none-eabi-objdump command not found. So just trying objdump would keep the behavior.

It should not do an $(error) otherwise it would fail even when not using objdump as we do export OBJDUMP.

@cladmi cladmi requested a review from jcarrano May 23, 2019 16:13
@smlng
Copy link
Member

smlng commented May 24, 2019

I'd say: make it consistent with the behaviour for objcopy just above.

@cladmi
Copy link
Contributor Author

cladmi commented May 24, 2019

I noticed that currently for objdump, the objdump usage already check for the command existing, so replacing it with false is not necessary and I can just set objdump as default. So was stupid to put false here as a similar behaviour of objcopy

RIOT/Makefile.include

Lines 596 to 598 in 35d43cc

objdump:
$(call check_cmd,$(OBJDUMP),Objdump program)
$(OBJDUMP) $(OBJDUMPFLAGS) $(ELFFILE) | less

objcopy is a bit different as it is always called even if not needed to try creating the .hex or .bin. So must not fail by default.

I will update to remove the false.

@smlng
Copy link
Member

smlng commented May 24, 2019

I will update to remove the false.

and maybe also add the alternative to look for gobjdump as done with (g)objcopy, on macOS (or other UNIXes) some might have installed the GNU variants instead.

@cladmi
Copy link
Contributor Author

cladmi commented May 24, 2019

Did not know thought that it could exist, I will.

@cladmi
Copy link
Contributor Author

cladmi commented May 24, 2019

I noticed something weird on my machine while testing this, if I was doing OBJDUMP ?= $(shell command -v $(PREFIX)objdump) it would result in make: command: Command not found but if I have $(shell command -v $(PREFIX)objdump || false) it would result in the expected output.

@cladmi
Copy link
Contributor Author

cladmi commented May 24, 2019

BOARD=samr21-xpro make --no-print-directory -C examples/hello-world/ info-debug-variable-OBJDUMP
/opt/gcc-arm-none-eabi-7-2018-q2-update/bin/arm-none-eabi-objdump
PATH=/usr/bin:/bin BOARD=samr21-xpro make --no-print-directory -C examples/hello-world/ info-debug-variable-OBJDUMP
/bin/sh: 1: arm-none-eabi-gcc: not found
objump

When I test for llvm-objdump I always endup with /usr/bin/llvm-objdump as it always is in my path.

@cladmi
Copy link
Contributor Author

cladmi commented May 27, 2019

@smlng Tell me if I should squash

Copy link
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

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

a small oops, otherwise it is OK.

makefiles/toolchain/llvm.inc.mk Outdated Show resolved Hide resolved
makefiles/toolchain/gnu.inc.mk Outdated Show resolved Hide resolved
@cladmi
Copy link
Contributor Author

cladmi commented May 27, 2019

Thanks I missed that one D:

@cladmi
Copy link
Contributor Author

cladmi commented May 27, 2019

I re-tested #11545 on top of the current changes and the testing procedure works.

@smlng
Copy link
Member

smlng commented May 28, 2019

please squash, if @jcarrano agrees 😄

@cladmi cladmi force-pushed the pr/make/objdump branch from 0d5eeb2 to 3ea3a6c Compare May 28, 2019 12:02
When '$(PREFIX)objdump' is not present fallback to native '(g)objdump'.
'objdump' is used when flashing for some boards but the toolchain may
not be installed when building in docker.

This will allow using 'objdump' in 'cpu/kinetis/dist/check-fcfield.sh'.
@cladmi cladmi force-pushed the pr/make/objdump branch from 3ea3a6c to f47b09f Compare May 28, 2019 12:04
@cladmi
Copy link
Contributor Author

cladmi commented May 28, 2019

Squashed first and rebased. It will allow basing #11545 on top for testing.

@jcarrano jcarrano merged commit bb6a06a into RIOT-OS:master May 28, 2019
@cladmi cladmi deleted the pr/make/objdump branch May 28, 2019 12:39
@cladmi
Copy link
Contributor Author

cladmi commented May 28, 2019

Thank you for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants