Attention is currently required from: Evan Benn.
Hello build bot (Jenkins), Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69267
to look at the new patch set (#11).
Change subject: tests: Detect llvm coverage run and redirect to real I/O functions
......................................................................
tests: Detect llvm coverage run and redirect to real I/O functions
Code coverage writes data to disk, we need to use real io functions at
this point so that the data is really written.
BUG=b:187647884
BRANCH=None
TEST=llvm-profdata merge -sparse default.profraw -o default.profdata
TEST=llvm-cov show ./flashrom_unit_tests
-instr-profile=default.profdata --format=html --output-dir=.
Change-Id: I21cc1d631e92fa19006b967e85676f108e80b307
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
---
M Documentation/building.md
M tests/io_real.c
M tests/tests.c
M tests/wraps.h
4 files changed, 50 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/69267/11
--
To view, visit https://review.coreboot.org/c/flashrom/+/69267
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I21cc1d631e92fa19006b967e85676f108e80b307
Gerrit-Change-Number: 69267
Gerrit-PatchSet: 11
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Thomas Heijligen, Evan Benn.
Hello build bot (Jenkins), Thomas Heijligen, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69266
to look at the new patch set (#11).
Change subject: tests: Detect gcov run and redirect to real I/O functions
......................................................................
tests: Detect gcov run and redirect to real I/O functions
Code coverage writes data to disk, we need to use real io functions at
this point so that the data is really written.
BUG=b:187647884
BRANCH=None
TEST=meson test
TEST=ninja coverage
Change-Id: If06053ecd78e886c8f7fc55813f4b5635be78c6b
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
---
M Documentation/building.md
M tests/tests.c
2 files changed, 32 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/66/69266/11
--
To view, visit https://review.coreboot.org/c/flashrom/+/69266
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If06053ecd78e886c8f7fc55813f4b5635be78c6b
Gerrit-Change-Number: 69266
Gerrit-PatchSet: 11
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Thomas Heijligen, Evan Benn.
Hello build bot (Jenkins), Thomas Heijligen, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69539
to look at the new patch set (#2).
Change subject: tests: Redirect to real I/O to support coverage run
......................................................................
tests: Redirect to real I/O to support coverage run
Implement a check that redirects mock io functions to the real
implementations. Real I/O functions are needed for the coverage tool to
be able to create and write files.
BUG=None
BRANCH=None
TEST=None
Change-Id: I0817fce6ea0f53a4c127794a0d8246504675f805
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
---
A tests/io_real.c
A tests/io_real.h
M tests/meson.build
M tests/wraps.h
4 files changed, 104 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/69539/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/69539
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I0817fce6ea0f53a4c127794a0d8246504675f805
Gerrit-Change-Number: 69539
Gerrit-PatchSet: 2
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk.
Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69620 )
Change subject: tests: Add selfcheck to unit tests
......................................................................
Patch Set 6:
(15 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69620/comment/bcb98b22_522cba6c
PS6, Line 7: Convert
> Replace Convert with Add. […]
Done
File board_enable.c:
https://review.coreboot.org/c/flashrom/+/69620/comment/f1d180e1_08c13711
PS6, Line 2527: const size_t board_matches_size = ARRAY_SIZE(board_matches);
> Why this can't be in the test code?
the size of the array is only known in the translation unit it is defined in
File tests/meson.build:
https://review.coreboot.org/c/flashrom/+/69620/comment/f6d42ec1_2eaba2ae
PS6, Line 25: well_formed
> I would rename all occurrences of "well_formed" into "selfcheck": file names, test methods names etc […]
Done
File tests/well_formed_tables.c:
https://review.coreboot.org/c/flashrom/+/69620/comment/dcdd942e_02a36d86
PS6, Line 23: name
> Maybe rename to prog_name?
its only prog_name for one of the callsites
https://review.coreboot.org/c/flashrom/+/69620/comment/22ff445a_ce150d65
PS6, Line 26: is false
> is false or NULL
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/6c50d1e5_8d6c15aa
PS6, Line 26: #assertion
> I assume this would print "p is false" which can be confusing. […]
#assertion is copied from what normal cmocka assertions print. We can do better.
https://review.coreboot.org/c/flashrom/+/69620/comment/2ff04049_0534353d
PS6, Line 24: do { \
: if (!(assertion)) \
: fail_msg(#assertion " is false for index:%zu name:%s", (index), \
: (name) ? (name) : "unknown"); \
: } while (0)
> I was also confused in the beginning, but then I thought this is probably a trick to avoid swallowin […]
yes its the standard semicolon trick for macros. So you can call it like a function.
https://review.coreboot.org/c/flashrom/+/69620/comment/db31623b_d14b0e8a
PS6, Line 33: (void)state; /* unused */
> Add a new line after `(void)state; /* unused */` and the same for all other test methods.
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/3f408137_585db4a4
PS6, Line 41: if (strcmp("internal", p->name) != 0)
: assert_table(p->devs.note, i, p->name);
> Original code has this check for all programmers with p->type == OTHER […]
the original code does this check for all types except 'default', and for default it does an unconditional assertion. I replaced that assertion with assert(p->type), although that is not as good as it no longer checks for non-valid values of the enum. I added that back in.
https://review.coreboot.org/c/flashrom/+/69620/comment/45140d0d_f5bc04d8
PS6, Line 51: assert_false(flashchips_size <= 1 || flashchips[flashchips_size - 1].name != NULL);
> Break down into two `assert_true`s
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/b2bc0543_4d49ff81
PS6, Line 58: }
> You missed `selfcheck_eraseblocks` functionality? […]
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/43193fb6_5e543a55
PS6, Line 66: assert_false(board_matches_size <= 1
: || board_matches[board_matches_size - 1].vendor_name != NULL);
> Break down into two `assert_true`s
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/83248581_22f7bf60
PS6, Line 71: assert_table(b->board_name, i, b->board_name);
: assert_table(b->board_name, i, b->board_name);
> The same line twice. […]
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/4da57f91_eb0b43fc
PS6, Line 83: SKIP_TEST
> Indent with tab
Done
https://review.coreboot.org/c/flashrom/+/69620/comment/2b68e0b2_33a8f7b6
PS6, Line 84: // CONFIG_INTERNAL
> /* comment */
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/69620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I41cd014d9bf909296b6c28e3e00548e6883ff41a
Gerrit-Change-Number: 69620
Gerrit-PatchSet: 6
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 24 Nov 2022 01:09:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Thomas Heijligen, Edward O'Callaghan, Evan Benn.
Hello build bot (Jenkins), Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69620
to look at the new patch set (#7).
Change subject: tests: Add selfcheck to unit tests
......................................................................
tests: Add selfcheck to unit tests
Add unit tests for programmer_table, flashchips, and board_matches
structs. The tests are derived from the selfcheck function, checking
that the required fields have been filled in.
BUG=b:140595239
BRANCH=None
TEST=meson test
Change-Id: I41cd014d9bf909296b6c28e3e00548e6883ff41a
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
---
M board_enable.c
M include/programmer.h
M tests/meson.build
A tests/selfcheck.c
M tests/tests.c
M tests/tests.h
6 files changed, 213 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/20/69620/7
--
To view, visit https://review.coreboot.org/c/flashrom/+/69620
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I41cd014d9bf909296b6c28e3e00548e6883ff41a
Gerrit-Change-Number: 69620
Gerrit-PatchSet: 7
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Thomas Heijligen, Evan Benn.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69268 )
Change subject: tests: Add llvm-cov option and run target for code coverage
......................................................................
Patch Set 11:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69268/comment/6b315a32_75a571f8
PS11, Line 14: TEST=meson test; ninja llvm-cov-tests
Can you test scenario when feature disables? when tests disabled?
Obviously, when feature disabled then coverage is not measured. The point of testing is to check flashrom builds and runs normally (without coverage and/or tests).
And please add those to the test scenarios in commit message. Thanks!
File meson.build:
https://review.coreboot.org/c/flashrom/+/69268/comment/12518494_58fff455
PS5, Line 534: if get_option('llvm_cov')
> coverage can be used independently of unit tests
I am confused, but what does it mean, coverage without running tests?
--
To view, visit https://review.coreboot.org/c/flashrom/+/69268
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56
Gerrit-Change-Number: 69268
Gerrit-PatchSet: 11
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-Comment-Date: Thu, 24 Nov 2022 00:41:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Angel Pons, Liam Flaherty.
Hello Felix Singer, build bot (Jenkins), Angel Pons, Liam Flaherty,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69933
to look at the new patch set (#3).
Change subject: tree/: Convert unlock func ptr into enumerate values
......................................................................
tree/: Convert unlock func ptr into enumerate values
By converting the blockprotect unlock function pointer
within the flashchip struct into enum values allows for
the flashchips db to be turn into pure, declarative data.
A nice side-effect of this is to reduce link-time symbol
space of chipdrivers and increase modularity of the
spi25_statusreg.c and related implementations.
BUG=none
TEST=ninja test.
Change-Id: Ie5c5db1b09d07e1a549990d6f5a622fae4c83233
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M 82802ab.c
M flashrom.c
M include/chipdrivers.h
M include/flash.h
M jedec.c
M spi25_statusreg.c
M tests/chip.c
M tests/chip_wp.c
8 files changed, 151 insertions(+), 73 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/69933/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/69933
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie5c5db1b09d07e1a549990d6f5a622fae4c83233
Gerrit-Change-Number: 69933
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Liam Flaherty <liamflaherty(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Liam Flaherty <liamflaherty(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Evan Benn.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69267 )
Change subject: tests: Detect llvm coverage file io and unmock io functions
......................................................................
Patch Set 10:
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69267/comment/f3e08fd3_587611ed
PS10, Line 7: tests: Detect llvm coverage file io and unmock io functions
Could you please re-use my comments for commit message from the previous patch into here, too? thanks!
File Documentation/building.md:
https://review.coreboot.org/c/flashrom/+/69267/comment/2097605a_aee8f9b5
PS10, Line 52:
: #### gcc / gcov
This line needs to be in a previous patch, where the section is added
File tests/unmock_io.c:
https://review.coreboot.org/c/flashrom/+/69267/comment/a4bc875d_4582a185
PS10, Line 32: (void)state;
can be dropped
https://review.coreboot.org/c/flashrom/+/69267/comment/e75d9d4c_14adf93e
PS10, Line 79: ".gcda"
Can this go into macro at the beginning of the file?
GCOV_COVERAGE_FILE
LLVM_COVERAGE_FILE
--
To view, visit https://review.coreboot.org/c/flashrom/+/69267
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I21cc1d631e92fa19006b967e85676f108e80b307
Gerrit-Change-Number: 69267
Gerrit-PatchSet: 10
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-Comment-Date: Thu, 24 Nov 2022 00:22:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Evan Benn.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69266 )
Change subject: tests: Detect gcov file io and unmock io functions
......................................................................
Patch Set 10:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69266/comment/6b019351_1d99152c
PS10, Line 7: Detect gcov file io and unmock io functions
Detect gcov run and redirect to real I/O functions
https://review.coreboot.org/c/flashrom/+/69266/comment/615a376c_626a43df
PS10, Line 9: we need to unmock the file io
we need to use real I/O functions
--
To view, visit https://review.coreboot.org/c/flashrom/+/69266
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If06053ecd78e886c8f7fc55813f4b5635be78c6b
Gerrit-Change-Number: 69266
Gerrit-PatchSet: 10
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-Comment-Date: Thu, 24 Nov 2022 00:18:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Angel Pons, Liam Flaherty.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69933 )
Change subject: tree/: Convert unlock func ptr into enumerate values
......................................................................
Patch Set 2:
This change is ready for review.
--
To view, visit https://review.coreboot.org/c/flashrom/+/69933
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie5c5db1b09d07e1a549990d6f5a622fae4c83233
Gerrit-Change-Number: 69933
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Reviewer: Liam Flaherty <liamflaherty(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Liam Flaherty <liamflaherty(a)chromium.org>
Gerrit-Comment-Date: Thu, 24 Nov 2022 00:16:35 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment