Hi Richard,
Thanks for the review!
The 09/27/2018 09:40, Richard Sandiford wrote:
> Tamar Christina <tamar.christ...@arm.com> writes:
> >
> > and no testsuite errors. Difference would depend on your site.exp.
> > On arm we get about 4500 new testcases and on aarch64 the low 10s.
> > On PowerPC and x86_64 no changes as expected since the default exp for these
> > just test the default configuration.
>
> Would be good to try --target_board unix{,-m32} on x86_64.
>
Running now.
> > What this means for new target checks is that they should always use either
> > check_cached_effective_target or check_cached_effective_target_indexed if
> > the
> > result of the check is to be cached.
> >
> > As an example the new vect_int looks like
> >
> > proc check_effective_target_vect_int { } {
> > return [check_cached_effective_target_indexed <name> {
> > expr {
> > <condition>
> > }}]
> > }
> >
> > The debug information that was once there is now all hidden in
> > check_cached_effective_target, (called from
> > check_cached_effective_target_indexed) and so the only thing you are
> > required to do is give it a unique cache name and a condition.
> >
> > The condition doesn't need to be an if statement so simple boolean
> > expressions are enough here:
> >
> > [istarget i?86-*-*] || [istarget x86_64-*-*]
> > || ([istarget powerpc*-*-*]
> > && ![istarget powerpc-*-linux*paired*])
> > || ...
> >
> > The expr may not be required as check_cached_effective_target forces
> > evaluation, but I have left these here just to be sure (TCL semantics
> > is confusing at times.).
>
> It's required, since "eval" just runs a tcl script (which can give back
> an arbitrary string) while "expr" evaluates the string as an expression.
>
> This means that the caching (as written) should work for arbitrary strings,
> such as command-line flags or the default value of -march.
>
Ah ok, got it,
> > @@ -120,22 +121,41 @@ proc check_cached_effective_target { prop args } {
> > global et_prop_list
> >
> > set target [current_target_name]
> > - if {![info exists et_cache($prop,target)]
> > - || $et_cache($prop,target) != $target} {
> > + if {![info exists et_cache($prop,$target)]} {
> > verbose "check_cached_effective_target $prop: checking $target" 2
> > - set et_cache($prop,target) $target
> > - set et_cache($prop,value) [uplevel eval $args]
> > + if {[string is true -strict $args] || [string is false -strict $args]} {
> > + set et_cache($prop,$target) $args
> > + } else {
> > + set et_cache($prop,$target) [uplevel eval $args]
> > + }
>
> Why are the checks for true and false needed? We shouldn't be
> using this function for something that's already true or false.
>
I had wanted to be a bit lenient here in accepting it if you have a function
that already
fully evaluated the expr before passing it to check_cached_effective_target.
i.e. something like
proc check_effective_target_vect_int { } {
return [check_cached_effective_target_indexed <name> [
expr {
<condition>
}]]
}
The error you would get if you do this is very confusing so I thought since it
didn't matter
much for the regexp only target triple tests that just accepting this would be
fine.
Should I drop it or keep it?
Thanks,
Tamar.
> > +# Implements a version of check_cached_effective_target that also takes
> > et_index
> > +# into account when creating the key for the cache.
> > +proc check_cached_effective_target_indexed { prop args } {
> > + global et_index
> > + set key "$et_index $prop"
> > + verbose "check_cached_effective_target_index $prop: returning $key" 2
> > +
> > + # Force the evaluation at this level since
> > check_cached_effective_target
> > + # may no longer be able to.
> > + if {[string is true -strict $args] || [string is false -strict $args]}
> > {
> > + set value $args
> > + } else {
> > + set value [uplevel eval $args]
> > + }
> > + return [check_cached_effective_target $key $value]
> > +}
>
> This defeats the point of the cache, since we'll evaluate the expression
> every time before passing it to check_cached_effective_target.
>
> How about:
>
> return [check_cached_effective_target $key [list uplevel eval $args]]
>
> > @@ -149,11 +169,13 @@ proc clear_effective_target_cache { } {
> > global et_cache
> > global et_prop_list
> >
> > + set target [current_target_name]
> > if {[info exists et_prop_list]} {
> > verbose "clear_effective_target_cache: $et_prop_list" 2
> > foreach prop $et_prop_list {
> > - unset et_cache($prop,value)
> > - unset et_cache($prop,target)
> > + if {[info exists et_cache($prop,$target)]} {
> > + unset et_cache($prop,$target)
> > + }
>
> Looks a bit odd that we now have to make this conditional. Maybe we
> should maintain an et_prop_list for each target. I.e.:
>
> if {![info exists et_prop_list]
> || [lsearch $et_prop_list $prop] < 0} {
> lappend et_prop_list $prop
> }
>
> in check_cached_effective_target would become an unconditional:
>
> lappend et_prop_list($target) $prop
>
> (since $prop should already be in the cache for $target if it's
> already in the list) and clear_effective_target_cache would then use:
>
> if {[info exists et_prop_list($target)]} {
> verbose "clear_effective_target_cache: $et_prop_list($target)" 2
> foreach prop $et_prop_list($target) {
> unset et_cache($prop,$target)
> }
> unset et_prop_list($target)
> }
>
> ...That said, I'm not sure what the point of et_prop_list actually is.
> The comment above clear_effective_target_cache says:
>
> # Clear effective-target cache. This is useful after testing
> # effective-target features and overriding TEST_ALWAYS_FLAGS and/or
> # ALWAYS_CXXFLAGS.
> # If one changes ALWAYS_CXXFLAGS or TEST_ALWAYS_FLAGS then they should
> # do a clear_effective_target_cache at the end as the target cache can
> # make decisions based upon the flags, and those decisions need to be
> # redone when the flags change. An example of this is the
> # asan_init/asan_finish pair.
>
> so at first I thought it was to avoid removing entries from before
> TEST_ALWAYS_FLAGS & co. had been changed, since those entries should
> still be valid after the flags have been reset. But that would only
> happen if something cleared et_prop_list before changing TEST_ALWAYS_FLAGS
> (so that et_prop_list only contains entries that postdate the change),
> and AFAICT nothing does.
>
> So I think we should simply remove et_prop_list and use an unconditional:
>
> array unset et_cache
>
> in clear_effective_target_cache.
>
> > @@ -380,12 +402,9 @@ proc check_visibility_available { what_kind } {
> > # be determined.
> >
> > proc check_alias_available { } {
> > - global alias_available_saved
> > global tool
> >
> > - if [info exists alias_available_saved] {
> > - verbose "check_alias_available returning saved
> > $alias_available_saved" 2
> > - } else {
> > + return [check_cached_effective_target alias_available {
> > set src alias[pid].c
> > set obj alias[pid].o
> > verbose "check_alias_available compiling testfile $src" 2
> > @@ -402,7 +421,7 @@ proc check_alias_available { } {
> >
> > if [string match "" $lines] then {
> > # No error messages, everything is OK.
> > - set alias_available_saved 2
> > + return 2
> > } else {
> > if [regexp "alias definitions not supported" $lines] {
> > verbose "check_alias_available target does not support
> > aliases" 2
> > @@ -411,24 +430,20 @@ proc check_alias_available { } {
> >
> > if { $objformat == "elf" } {
> > verbose "check_alias_available but target uses ELF format,
> > so it ought to" 2
> > - set alias_available_saved -1
> > + return -1
> > } else {
> > - set alias_available_saved 0
> > + return 0
> > }
> > } else {
> > if [regexp "only weak aliases are supported" $lines] {
> > verbose "check_alias_available target supports only weak
> > aliases" 2
> > - set alias_available_saved 1
> > + return 1
> > } else {
> > - set alias_available_saved -1
> > + return -1
> > }
> > }
> > }
> > -
> > - verbose "check_alias_available returning $alias_available_saved" 2
> > - }
> > -
> > - return $alias_available_saved
> > + }]
> > }
>
> As it stands, these should be "expr" rather than "return".
> "return" will make check_cached_effective_target return at the point of
> the uplevel eval, bypassing the actual caching.
>
> If instead you want to support "return" (would be nice IMO), you could
> use something like:
>
> set code [catch {uplevel eval $args} result options]
> if {$code != 0 && $code != 2} {
> return -code $code $result
> }
> set et_cache($prop,$target) $result
>
> Thanks,
> Richard
--
