Bug 646856 – flag-o-matic.eclass: clean strip-flags up

• Product: Gentoo Linux
• Status: RESOLVED INVALID

[Description kfm 2018-02-07 02:53:40 UTC]

The -O2 fallback code does in strip-flags() does not work correctly because the name of the "new" array cannot be dereferenced by the _is_flagq() function. Please review and apply the attached patches, which contains far more detail on the issue, as well as a fix.

There are two patches, one of which cleans up the strip-flags function, and the other of which cleans up the _is_flagq() function. These patches may also be viewed here: https://github.com/kerframil/gentoo/commits/stable.

[Comment 1 kfm 2018-02-07 02:55:37 UTC]

Created attachment 518314 [details, diff]
bug-646856-01.patch

Patch for strip-flags().

[Comment 2 kfm 2018-02-07 02:56:22 UTC]

Created attachment 518316 [details, diff]
bug-646856-02.patch

Patch for _is_flagq().

[Comment 3 Sergei Trofimovich (RETIRED) 2018-02-07 07:36:39 UTC]

> The -O2 fallback code does in strip-flags() does not work correctly because the name of the "new" array cannot be dereferenced by the _is_flagq() function.

What was the input CFLAGS you were testing, what was the expected result and the output?

You can try to add a test to eclass/tests/flag-o-matic.sh to get the idea about the case you are fixing.

[Comment 4 kfm 2018-02-07 12:48:51 UTC]

Thanks for pointing me to the formal tests, Sergei; I'd entirely forgotten that they existed. Unfortunately, it doesn't matter because I was wrong about the -O2 fallback path being non-functional! I swear that I tested my presumptions (regarding function-local scope) before posting, but my own test case was clearly incorrect and led me to an erroneous conclusion.

I knew - but forgot somewhere along the line - that bash uses dynamic scoping, whereby a variable that isn't local to the current function is attempted to be resolved in the previous function, all the way up the stack to the global scope. So, the indirect referencing in _is_flagq works for its intended use case and, technically, this bug is INVALID. Sorry about that.

That said, I still think that the existing code is hairy and does rely very heavily upon word splitting and potentially unsafe expansions that are trivially avoided (the flexibility of read under bash is a great asset). For what its worth, my code does pass all of the existing flag-o-matic tests, and is safer and faster. If anyone feels like incorporating it then great. Otherwise, c'est la vie.