Skip to contentSkip to conversation

Makefile: use $(shell ...) for determining file_size

PATCH Superseded Chris Packham 5 participants 12 messages Jul 23, 2014 – Aug 10, 2014 Kbuild

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Chris Packham

author
#0 lore raw

PATCH 1/1 Makefile: use $(shell ...) for determining file_size

file_size was being calculated using back-ticks but map_size uses $(shell ...). Update the file_size calculation to use $(shell ...).

Signed-off-by Chris Packham <[email protected]>

---
The back ticks didn't work in my environment (GNU Make 3.81). Updating to use $(shell ...) makes sense from a consistency view even if the problem is my environment.

 Makefile |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Makefile +1 -1
index ca212b5..f6a0f68 100644
@@ -786,7 +786,7 @@ u-boot.hex u-boot.srec: u-boot FORCE
 OBJCOPYFLAGS_u-boot.bin := -O binary
 
 binary_size_check: u-boot.bin System.map FORCE
-	@file_size=`stat -c %s u-boot.bin` ; \
+	file_size=$(shell stat -c %s u-boot.bin) ; \
 	map_size=$(shell cat System.map | \
 		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end = $$1} END {if (start != "" && end != "") print "ibase=16; " toupper(end) " - " toupper(start)}' \
 		| bc); \
Signature · 1 line
1.7.9.5
#2 lore raw

PATCH 1/1 Makefile: use $(shell ...) for determining file_size

Quoted 11 lines · Simon Glass
Quoted 4 lines · Chris Packham

file_size was being calculated using back-ticks but map_size uses $(shell ...). Update the file_size calculation to use $(shell ...).

Signed-off-by Chris Packham <[email protected]>
Acked-by Simon Glass <[email protected]>

But you might want to look at this.

http://patchwork.ozlabs.org/patch/371936/

Yeah, Jeroen, can we get a v2 of your patch that uses $(shell ... )? Thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140723/b9d9fd8c/attachment.pgp>
#3 lore raw

PATCH 1/1 Makefile: use $(shell ...) for determining file_size

Hi,

Quoted 5 lines · Tom Rini
Quoted 4 lines · Simon Glass
Quoted 3 lines · Chris Packham

file_size was being calculated using back-ticks but map_size uses $(shell ...). Update the file_size calculation to use $(shell ...).

Quoted 10 lines
Quoted 6 lines
Quoted 1 line
Signed-off-by Chris Packham <[email protected]>
Acked-by Simon Glass <[email protected]>

But you might want to look at this.

http://patchwork.ozlabs.org/patch/371936/

Yeah, Jeroen, can we get a v2 of your patch that uses $(shell ... )? Thanks!

no problem. For the record I have not seen any issues with mentioned gmake version. But using $(shell .. ) seem like a sane thing to do. v2 is on its way, running MAKEALL for arm (can we keep it?).

Chris: blackfin targets seem to rely on the same construct btw...

Regards,
Jeroen

#4 lore raw

PATCH 1/1 Makefile: use $(shell ...) for determining file_size

Quoted 13 lines · Jeroen Hofstee

Hi,

Quoted 10 lines · Tom Rini
Quoted 9 lines · Simon Glass
Quoted 4 lines · Chris Packham

file_size was being calculated using back-ticks but map_size uses $(shell ...). Update the file_size calculation to use $(shell ...).

Signed-off-by Chris Packham <[email protected]>
Acked-by Simon Glass <[email protected]>

But you might want to look at this.

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

http://patchwork.ozlabs.org/patch/371936/

Yeah, Jeroen, can we get a v2 of your patch that uses $(shell ... )? Thanks!

no problem. For the record I have not seen any issues with mentioned gmake version. But using $(shell .. ) seem like a sane thing to do. v2 is on its way, running MAKEALL for arm (can we keep it?).

MAKEALL isn't going away for v2014.10 but please reply to Simon's RFC about it. I do have to admit that switching all of my stuff to buildman is still on my TODO list.

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140723/64798183/attachment.pgp>
author
#5 lore raw

PATCH 1/1 Makefile: use $(shell ...) for determining file_size

In reply to Simon Glass

Hi Simon,

Quoted 4 lines · Simon Glass
Quoted 3 lines · Chris Packham

file_size was being calculated using back-ticks but map_size uses $(shell ...). Update the file_size calculation to use $(shell ...).

Quoted 8 lines
Quoted 1 line
Signed-off-by Chris Packham <[email protected]>
Acked-by Simon Glass <[email protected]>

But you might want to look at this.

http://patchwork.ozlabs.org/patch/371936/

Thanks. I've re-submitted a version with Jeroen's change included and it seems a v2 of his patch is imminent too. Either way I'm not fussed.

But this has highlighted another issue I'm experiencing related to this code. Originally I thought it was because I was doing something a bit weird with a custom u-boot.lds but actually the problem is my u-boot is setup so that it finishes exactly at the end of a 32bit address space so _image_binary_end is just off the end of it which screws up the size calculation.

  $ grep _image_binary_end u-boot.map
                0x0000000100000000                _image_binary_end = .
  $ grep _image_binary_end System.map
  00000000 A _image_binary_end
  $ make binary_size_check
  ...
  System.map shows a binary size of -4294180864
    but u-boot.bin shows 786432
  make: *** [binary_size_check] Error 1

I think it should be possible to change binary_size_check to use u-boot.map instead of System.map but would that be OK for all architectures?

author
#6 lore raw

PATCH 1/1 Makefile: use u-boot.map for binary_size_check

u-boot.map is generated automatically by the compiler and more importantly can handle addresses >4GB.
---

Quoted 38 lines · Chris Packham

Hi Simon,

Quoted 4 lines · Simon Glass
Quoted 3 lines · Chris Packham

file_size was being calculated using back-ticks but map_size uses $(shell ...). Update the file_size calculation to use $(shell ...).

Quoted 8 lines
Quoted 1 line
Signed-off-by Chris Packham <[email protected]>
Acked-by Simon Glass <[email protected]>

But you might want to look at this.

http://patchwork.ozlabs.org/patch/371936/

Thanks. I've re-submitted a version with Jeroen's change included and it seems a v2 of his patch is imminent too. Either way I'm not fussed.

But this has highlighted another issue I'm experiencing related to this code. Originally I thought it was because I was doing something a bit weird with a custom u-boot.lds but actually the problem is my u-boot is setup so that it finishes exactly at the end of a 32bit address space so _image_binary_end is just off the end of it which screws up the size calculation.

  $ grep _image_binary_end u-boot.map
                0x0000000100000000                _image_binary_end = .
  $ grep _image_binary_end System.map
  00000000 A _image_binary_end
  $ make binary_size_check
  ...
  System.map shows a binary size of -4294180864
    but u-boot.bin shows 786432
  make: *** [binary_size_check] Error 1
Quoted 3 lines

I think it should be possible to change binary_size_check to use u-boot.map instead of System.map but would that be OK for all architectures?

Something like this works for me but maybe there is a way to get nm to handle addresses >4GB.

 Makefile |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
Makefile +4 -3
index b98a80a..3ed02a4 100644
@@ -785,14 +785,15 @@ u-boot.hex u-boot.srec: u-boot FORCE
 
 OBJCOPYFLAGS_u-boot.bin := -O binary
 
-binary_size_check: u-boot.bin System.map FORCE
+binary_size_check: u-boot.bin FORCE
 	@file_size=$(shell wc -c u-boot.bin | awk '{print $$1}') ; \
-	map_size=$(shell cat System.map | \
+	map_size=$(shell cat u-boot.map | \
 		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end = $$1} END {if (start != "" && end != "") print "ibase=16; " toupper(end) " - " toupper(start)}' \
+		| sed 's/0X//g' \
 		| bc); \
 	if [ "" != "$$map_size" ]; then \
 		if test $$map_size -ne $$file_size; then \
-			echo "System.map shows a binary size of $$map_size" >&2 ; \
+			echo "u-boot.map shows a binary size of $$map_size" >&2 ; \
 			echo "  but u-boot.bin shows $$file_size" >&2 ; \
 			exit 1; \
 		fi \
Signature · 1 line
1.7.9.5
#7 lore raw

PATCH 1/1 Makefile: use u-boot.map for binary_size_check

Dear Chris Packham,

Quoted 2 lines · In message <[email protected]> you wrote:

u-boot.map is generated automatically by the compiler and more importantly can handle addresses >4GB.

...

Quoted 3 lines
Quoted 3 lines

I think it should be possible to change binary_size_check to use u-boot.map instead of System.map but would that be OK for all architectures?

...

Quoted 2 lines

Something like this works for me but maybe there is a way to get nm to handle addresses >4GB.

...

Makefile u-boot.hex u-boot.srec: u-boot FORCE · line 792

Quoted 3 lines
Makefile +2 -0
+	map_size=$(shell cat u-boot.map | \
 		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end = $$1} END {if (start != "" && end != "") print "ibase=16; " toupper(end) " - " toupper(start)}' \
+		| sed 's/0X//g' \

Do we _really_ need all of this?

This looks very much like creaping featurism to me...

Best regards,

Wolfgang Denk

Signature · 6 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Remember, Information is not knowledge;  Knowledge  is  not  Wisdom;
Wisdom is not truth; Truth is not beauty; Beauty is not love; Love is
not music; Music is the best."                          - Frank Zappa
#8 lore raw

PATCH 1/1 Makefile: use u-boot.map for binary_size_check

Quoted 13 lines · Wolfgang Denk

Dear Chris Packham,

Quoted 2 lines · In message <[email protected]> you wrote:

u-boot.map is generated automatically by the compiler and more importantly can handle addresses >4GB.

...

Quoted 3 lines
Quoted 3 lines

I think it should be possible to change binary_size_check to use u-boot.map instead of System.map but would that be OK for all architectures?

...

Quoted 2 lines

Something like this works for me but maybe there is a way to get nm to handle addresses >4GB.

...

Quoted 8 lines

Makefile u-boot.hex u-boot.srec: u-boot FORCE · line 792

Quoted 3 lines
Makefile +2 -0
+	map_size=$(shell cat u-boot.map | \
 		awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end = $$1} END {if (start != "" && end != "") print "ibase=16; " toupper(end) " - " toupper(start)}' \
+		| sed 's/0X//g' \

Do we _really_ need all of this?

This looks very much like creaping featurism to me...

So it's a safetey check. The various linker script clean-ups we've been doing have introduced a problem from time to time and binary_check_size gives a sanity check. This just fixes a corner case in the test.

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140724/89be7428/attachment.pgp>
author
#9 lore raw

PATCH 1/1 Makefile: use u-boot.map for binary_size_check

Hi Wolfgang, Tom,

Quoted 14 lines · Tom Rini
Quoted 13 lines · Wolfgang Denk

Dear Chris Packham,

Quoted 2 lines · In message <[email protected]> you wrote:

u-boot.map is generated automatically by the compiler and more importantly can handle addresses >4GB.

...

Quoted 3 lines
Quoted 3 lines

I think it should be possible to change binary_size_check to use u-boot.map instead of System.map but would that be OK for all architectures?

...

Quoted 2 lines

Something like this works for me but maybe there is a way to get nm to handle addresses >4GB.

...

Quoted 8 lines
Quoted 8 lines

Makefile u-boot.hex u-boot.srec: u-boot FORCE · line 792

Quoted 3 lines
Makefile +2 -0
+   map_size=$(shell cat u-boot.map | \
            awk '/_image_copy_start/ {start = $$1} /_image_binary_end/ {end = $$1} END {if (start != "" && end != "") print "ibase=16; " toupper(end) " - " toupper(start)}' \
+           | sed 's/0X//g' \

Do we _really_ need all of this?

This looks very much like creaping featurism to me...

Do we need it? Maybe not I'd defer that judgement to you.

My problem is binary_size_check is on by default and my particular platform (an out of tree ARM board) fails to build because of it. If there was a way of opting out I'd happily use it.

Quoted 4 lines

So it's a safetey check. The various linker script clean-ups we've been doing have introduced a problem from time to time and binary_check_size gives a sanity check. This just fixes a corner case in the test.

I wouldn't say this is a corner case. Certainly for the ARM target I'm building it is unusual but I know plenty of powerpc boards would hit the same problem becuase they have a jump instruction at the top of their address space.

Quoted 3 lines

--
Tom

#10 lore raw

[U-Boot] Makefile: use u-boot.map for binary_size_check

In reply to Chris Packham
Quoted 2 lines · Chris Packham

u-boot.map is generated automatically by the compiler and more importantly can handle addresses >4GB.

Applied to u-boot/master, thanks!

Signature · 8 lines
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140810/8427baea/attachment.pgp>
#11 lore raw

PATCH 1/1 Makefile: use $(shell ...) for determining file_size

In reply to Chris Packham

Hi Chris,

Quoted 38 lines · Chris Packham

Hi Simon,

Quoted 4 lines · Simon Glass
Quoted 3 lines · Chris Packham

file_size was being calculated using back-ticks but map_size uses $(shell ...). Update the file_size calculation to use $(shell ...).

Quoted 8 lines
Quoted 1 line
Signed-off-by Chris Packham <[email protected]>
Acked-by Simon Glass <[email protected]>

But you might want to look at this.

http://patchwork.ozlabs.org/patch/371936/

Thanks. I've re-submitted a version with Jeroen's change included and it seems a v2 of his patch is imminent too. Either way I'm not fussed.

But this has highlighted another issue I'm experiencing related to this code. Originally I thought it was because I was doing something a bit weird with a custom u-boot.lds but actually the problem is my u-boot is setup so that it finishes exactly at the end of a 32bit address space so _image_binary_end is just off the end of it which screws up the size calculation.

  $ grep _image_binary_end u-boot.map
                0x0000000100000000                _image_binary_end = .
  $ grep _image_binary_end System.map
  00000000 A _image_binary_end
  $ make binary_size_check
  ...
  System.map shows a binary size of -4294180864
    but u-boot.bin shows 786432
  make: *** [binary_size_check] Error 1
Quoted 3 lines

I think it should be possible to change binary_size_check to use u-boot.map instead of System.map but would that be OK for all architectures?

That's unfortunate - it's just a little check. It does protect again a breakage but we need to keep it simple. Maybe in this case we can mask with 0xffffffff and get the right answer?

Regards,
Simon