Usage of $(eval) considered harmful.

[Brian Maher]

I think it is great that at last we have a re-usable make library. Thanks for starting this project!

Unfortunately, it looks like all of the usages of $(eval) do not properly take into account the fact that a parameter might contain $$. Here is a simple example of how tr fails:

$(call test_assert,$(call tr,$$ %,D p,$$ollar %ercent),Dollar percent)

Which results in this error:

Testing 'tr': ...X ERROR 'llar percent' != 'Dollar percent'

The only fix for this that I'm aware of is to recursively build the code that you want executed as such:

tr = $(__gmsl_tr3)$(eval _tr.t:=$(call _tr.gen,$1))$(_tr.t)

_tr.gen=$(if $1, \ $$(subst $$(word $(words $1),$$(value 1)),$(strip \ $$(word $(words $1),$$(value 2))),$(strip \ $(call _tr.gen,$(call rest,$1)))), \ $$(value 3))

Similarly the strlen function has the same problem:

$(call test_assert,$(call strlen,$$$$xx%%),6)

Here is my suggested fix... simply create the big $(subst <character>,x ,<recurse>) expression to translate the string into a bunch of space separated x(s)... notice that we now need to make sure that the "x" is also subst'ed (so the spaces can be added). We also need to escape some special characters:

define __gmsl_newline

endef
gmsl_tab := $(subst,,) #
gmsl_backslash := $(subst x,,\x)
gmsl_comma := ,
gmsl_pound := $(subst \,,#)
gmsl_space := $(subst,,) #
gmsl_paren_o := (
gmsl_paren_c := )
gmsl_characters := A B C D E F G H I J K L M N O P Q R S T U V W X Y Z
gmsl_characters += a b c d e f g h i j k l m n o p q r s t u v w x y z
gmsl_characters += 0 1 2 3 4 5 6 7 8 9
gmsl_characters += ` ~ ! @ % ^ & * - _ = +
gmsl_characters += { } [ ] \ : ; ' " < > . / ? |
gmsl_characters += $$(gmsl_pound) $$$$ $$(gmsl_comma)
gmsl_characters += $$(gmsl_paren_o) $$(gmsl_paren_c)
gmsl_characters += $$(gmsl_space) $$(gmsl_newline) $$(gmsl_tab)

` # <-- keep emacs happy

We generate the code so it looks like this:

$(subst A,x ,<recurse>)

The base case is $(value 1)

_strlen.gen=$(if $1, \ $$(subst $(firstword $1),x ,$(strip \ $(call _strlen.gen,$(call rest,$1)))),$(strip \ $$(value 1)))

Make bug?? We need to terminate the endif before eval is executed...

but this kind of defeats the purpose of the conditional...

endif # __gmsl_have_eval

$(eval strlen = $$(gmsl_tr1)$$(words $(strip \ $(call _strlen.gen,$(gmsl_characters)))))

[Brian Maher]

Here are fixes for all usages of $(eval) [excluding the test file that also has issues]. Note that I use _splitchar.gen for fixing both strlen and substr functions. Fixing push/pop/set was much easier, simply needed to do some extra escaping. It should also be noted that many characters can not be used as keys to the associative array (space, pound, left paren, right paren, dollar to name a few).

tr = $(__gmsl_tr3)$(eval _tr.t:=$(call _tr.gen,$1))$(_tr.t)

_tr.gen=$(if $1, \ $$(subst $$(word $(words $1),$$1),$(strip \ $$(word $(words $1),$$2)),$(strip \ $(call _tr.gen,$(call rest,$1)))), \ $$3)

define __gmsl_newline

endef
gmsl_tab := $(subst ,,) #
gmsl_space := $(subst ,,) #
gmsl_pound := $(subst \,,#)
gmsl_backslash := $(subst x,,)
gmsl_comma := ,
gmsl_paren_o := (
gmsl_paren_c := )
gmsl_characters := A B C D E F G H I J K L M N O P Q R S T U V W X Y Z
gmsl_characters += a b c d e f g h i j k l m n o p q r s t u v w x y z
gmsl_characters += 0 1 2 3 4 5 6 7 8 9
gmsl_characters += ` ~ ! @ % ^ & * - _ = +
gmsl_characters += { } [ ] \ : ; ' " < > . / ? |
gmsl_characters += $$(gmsl_pound) $$$$ $$(gmsl_comma)
gmsl_characters += $$(gmsl_paren_o) $$(gmsl_paren_c)

_splitchar.gen=$(strip $(if $1, \ $$(subst $(firstword $1),$(firstword $1) ,$(call \ _splitchar.gen,$(call rest,$1))), \ $$(subst $$(gmsl_newline),§§§ ,$$(subst \ $$(gmsl_tab),§§ ,$$(subst \ $$(__gmsl_space),§ ,$$1)))))

$(eval strlen=$$(gmsl_tr1)$$(words \ $(call _splitchar.gen,$(gmsl_characters))))

$(eval substr=$$(gmsl_tr3)$$(subst §, ,$$(subst $$(gmsl_space),,$(strip \ $$(subst §§,$$(gmsl_tab),$$(subst §§§,$$(gmsl_newline),$$(wordlist \ $$2,$$3,$(call _splitchar.gen,$(__gmsl_characters)))))))))

push = $(gmsl_tr2)$(eval gmsl_stack_$1 := $$2 $(if $(filter-out undefined,\ $(origin gmsl_stack_$1)),$$(gmsl_stack_$1)))

pop = $(gmsl_tr1)$(strip $(if $(filter-out undefined,$(origin
gmsl_stack_$1)), \ $(call first,$(gmsl_stack_$1)) \ $(eval gmsl_stack_$1 := $$(call rest,$$(__gmsl_stack_$1)))))

set = $(gmsl_tr3)$(eval gmsl_aa_$1_$2 = $$3)

[John Graham-Cumming]

Thanks.

I've added this to my GMSL TODO list and I'll get to it (plus updating the test suite) in the new year.

John.

[Brian Maher]

Actually, that strlen implementation I just posted is also flawed. Here is a test that shows the flaw:

$(call test_assert,$(call strlen,$$$$xx$(gmsl_tab)$(gmsl_tab)%%),8)

Now I see why the original implementation replaced all characters with "x" then replaced all "x"s with "x "... if we change this implementation to follow in the spirit of the original implementation then things work correctly again:

_strlen.gen=$(if $1, \ $$(subst $(firstword $1),x,$(strip \ $(call _strlen.gen,$(call rest,$1)))),$(strip \ $$(value 1)))
endif # gmsl_have_eval
$(eval strlen = $$(gmsl_tr1)$$(words $(strip \ $$(subst x,x ,$(call _strlen.gen,$(__gmsl_characters))))))

We can then also remove "x" from __gmsl_characters.

[Brian Maher]

Note that other usages of $(eval) have the same subtle bug. For example, substr fails this test:

$(call test_assert,$(call substr,$$$$xxk$(gmsl_tab)$(gmsl_tab)%%,5,6),k)

And push/pop fails this test:

$(call push,mystack,$$f1)
$(call test_assert,$(call pop,mystack),$$f1)

In fact even test_assert doesn't work properly in the above test (prints 1 != 1).

[John Graham-Cumming]

Thanks for the feedback. You are, of course, correct that the $$ is getting $(eval)ed which is messing things up.

I don't have time to go through the GMSL code and fix all the instances, do you?

For the time being I'm going to check in code that will cause a fatal error if any of the arguments to the GMSL functions have a $ in them.

John.