svn commit: r306806 - head/usr.bin/dtc

[Conrad Meyer]

Hi Ed,

I suspect this commit accidentally introduced a bug in define handling
and was detected by Coverity as dead code (CID 1376409).

On Fri, Oct 7, 2016 at 5:57 AM, Ed Maste <emaste at freebsd.org> wrote:
> Author: emaste
> Date: Fri Oct  7 12:57:35 2016
> New Revision: 306806
> URL: https://svnweb.freebsd.org/changeset/base/306806
>
> Log:
>   Improvements to BSD-licensed DTC.
>
>   - Numerous crash and bug fixes
>   - Improved warning and error messages
>   - Permit multiple labels on nodes and properties
>   - Fix node at address references
>   - Add support for /delete-node/
>   - Consume whitespace after a node
>   - Read the next token before the second /memreserve/
>   - Fix parsing of whitespace
>   - Clean up /delete-node/ and add support for /delete-property/
>   - Handle /delete-node/ specifying a unit address
>
>   Obtained from:        https://github.com/davidchisnall/dtc @df5ede4
>
> Modified:
>   head/usr.bin/dtc/checking.cc
>   head/usr.bin/dtc/checking.hh
>   head/usr.bin/dtc/dtb.cc
>   head/usr.bin/dtc/dtb.hh
>   head/usr.bin/dtc/dtc.1
>   head/usr.bin/dtc/dtc.cc
>   head/usr.bin/dtc/fdt.cc
>   head/usr.bin/dtc/fdt.hh
>   head/usr.bin/dtc/input_buffer.cc
>   head/usr.bin/dtc/input_buffer.hh
>   head/usr.bin/dtc/string.cc
>   head/usr.bin/dtc/util.hh

The original diff email was truncated, but this function was both
moved from fdt.cc to input_buffer.cc and modified at the same time:

fdt.cc:
@@ -1169,87 +1327,10 @@
  }
 }

-bool
-device_tree::parse_include(input_buffer &input,
-                        const std::string &dir,
-                        std::vector<node_ptr> &roots,
-                        FILE *depfile,
-                        bool &read_header)
-{
- if (!input.consume("/include/"))
- {
- return false;
- }
- bool reallyInclude = true;
- if (input.consume("if "))
- {
- input.next_token();
- string name = string::parse_property_name(input);
- // XXX: Error handling
- if (defines.find(name) == defines.end())
- {
- reallyInclude = false;
- }
- input.consume('/');
- }
- input.next_token();
- if (!input.consume('"'))
- {
- input.parse_error("Expected quoted filename");
- valid = false;
- return false;
- }
- int length = 0;
- while (input[length] != '"') length++;
-
- std::string file((const char*)input, length);
- std::string include_file = dir + '/' + file;
- input.consume(file.c_str());
- if (!reallyInclude)
- {
- input.consume('"');
- input.next_token();
- return true;
- }
...

input_buffer.cc:

+void
+text_input_buffer::handle_include()
+{
+ bool reallyInclude = true;
+ if (consume("if "))
+ {
+ next_token();
+ string name = parse_property_name();
+ if (defines.count(name) > 0)
+ {
+ reallyInclude = true;
+ }
+ consume('/');
+ }
+ next_token();
+ if (!consume('"'))
+ {
+ parse_error("Expected quoted filename");
+ return;
+ }
+ string file = parse_to('"');
+ consume('"');
+ if (!reallyInclude)
+ {
+ return;
+ }

Note the use of the 'reallyInclude' sentinel.  In the old function, it
would be false if a "define" wasn't found in the "defines" lookup
table, causing an early function return.

In the new function, it is only ever initialized to true — even if the
"define" isn't located in the global "defines" lookup table.

Maybe the sense of the 'if (defines.count(name) > 0)' check and set
should be inverted?  Something like:

if (defines.count(name) <= 0)
   reallyInclude = false;

Best,
Conrad