- flashrom-gerrit - mail.coreboot.org

[M] Change in flashrom[master]: flashrom: Check for flash access restricitons erase path
by Nikolai Artemiev (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Edward O'Callaghan. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70517 ) Change subject: flashrom: Check for flash access restricitons erase path ...................................................................... Patch Set 1: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70517/comment/f3e5f60a_b42bcfce PS1, Line 11: will not erase anythin This isn't implemented yet. We will need to iterate regions and check for any protected regions before we start writing so we can bail out before we change anythibng. -- To view, visit https://review.coreboot.org/c/flashrom/+/70517 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If027a96a024782c7707c6d38680709a1a117f3ef Gerrit-Change-Number: 70517 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Fri, 09 Dec 2022 04:43:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom: Check for flash access restricitons erase path
by Nikolai Artemiev (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Edward O'Callaghan. Hello Edward O'Callaghan, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/70517 to review the following change. Change subject: flashrom: Check for flash access restricitons erase path ...................................................................... flashrom: Check for flash access restricitons erase path Skip unwritable regions if FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS is true. If the flag is false, erase operations that include an unwritable region will not erase anything and return an error. BUG=b:260440773 BRANCH=none TEST=flashrom -E on dedede (JSL) Change-Id: If027a96a024782c7707c6d38680709a1a117f3ef CoAuthored-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashrom.c 1 file changed, 53 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/17/70517/1 diff --git a/flashrom.c b/flashrom.c index 7a77845..23af5da 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1410,13 +1410,41 @@ all_skipped = false; msg_cdbg("E"); - if (erasefn(flashctx, info->erase_start, erase_len)) - goto _free_ret; - if (check_erased_range(flashctx, info->erase_start, erase_len)) { - msg_cerr("ERASE FAILED!\n"); - goto _free_ret; + + unsigned int len; + for (unsigned int addr = info->erase_start; addr < info->erase_start + erase_len; addr += len) { + struct flash_region region; + get_flash_region(flashctx, addr, &region); + + len = min(info->erase_start + erase_len, region.end) - addr; + + if (region.write_prot) { + if (flashctx->flags.skip_unwritable_regions) { + msg_gdbg("%s: cannot erase inside %s region (%#08x..%#08x), skipping range (%#08x..%#08x).\n", + __func__, region.name, region.start, region.end - 1, addr, addr + len - 1); + free(region.name); + continue; + } + + msg_gerr("%s: cannot erase inside %s region (%#08x..%#08x).\n", + __func__, region.name, region.start, region.end - 1); + free(region.name); + return -1; + } + + msg_gdbg("%s: %s region (%#08x..%#08x) is writable, erasing range (%#08x..%#08x).\n", + __func__, region.name, region.start, region.end - 1, addr, addr + len - 1); + free(region.name); + + if (erasefn(flashctx, addr, len)) + goto _free_ret; + if (check_erased_range(flashctx, addr, len)) { + msg_cerr("ERASE FAILED!\n"); + goto _free_ret; + } } + if (region_unaligned) { unsigned int starthere = 0, lenhere = 0, writecount = 0; /* get_next_write() sets starthere to a new value after the call. */ -- To view, visit https://review.coreboot.org/c/flashrom/+/70517 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If027a96a024782c7707c6d38680709a1a117f3ef Gerrit-Change-Number: 70517 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: flashrom: Check for flash access restricitons write_flash()
by Nikolai Artemiev (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Edward O'Callaghan. Hello Edward O'Callaghan, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/70516 to review the following change. Change subject: flashrom: Check for flash access restricitons write_flash() ...................................................................... flashrom: Check for flash access restricitons write_flash() Make write_flash() skip unwritable regions if FLASHROM_FLAG_SKIP_UNREADABLE_REGIONS is true. If the flag is false write operations that include an unwritable region will not write anything and return an error. BUG=b:260440773 BRANCH=none TEST=flashrom -w on dedede (JSL) Change-Id: Idacf0d5218da9d9929f4877fc7665fe608b87fe0 CoAuthored-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashrom.c 1 file changed, 72 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/16/70516/1 diff --git a/flashrom.c b/flashrom.c index 76a2801..7a77845 100644 --- a/flashrom.c +++ b/flashrom.c @@ -932,10 +932,58 @@ return NULL; } -static int write_flash(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) +/* + * write_flash - wrapper for flash->write() with additional high-level policy + * + * @param flash flash chip + * @param buf buffer to write to flash + * @param start start address in flash + * @param len number of bytes to write + * @return 0 on success, + * -1 if any write fails. + * + * This wrapper simplifies most cases when the flash chip needs to be written + * since policy decisions such as non-fatal error handling is centralized. + */ +static int write_flash(struct flashctx *flash, const uint8_t *buf, + unsigned int start, unsigned int len) { - write_func_t *write_func = lookup_write_func_ptr(flash->chip); - return write_func(flash, buf, start, len); + unsigned int write_len; + for (unsigned int addr = start; addr < start + len; addr += write_len) { + struct flash_region region; + get_flash_region(flash, addr, &region); + + write_len = min(start + len, region.end) - addr; + const uint8_t *rbuf = buf + addr - start; + + if (region.write_prot) { + if (flash->flags.skip_unwritable_regions) { + msg_gdbg("%s: cannot write inside %s region (%#08x..%#08x), skipping (%#08x..%#08x).\n", + __func__, region.name, region.start, region.end - 1, addr, addr + write_len - 1); + continue; + } else { + msg_gerr("%s: cannot write inside %s region (%#08x..%#08x).\n", + __func__, region.name, region.start, region.end - 1); + free(region.name); + return -1; + } + } + + msg_gdbg("%s: %s region (%#08x..%#08x) is writable, writing range (%#08x..%#08x).\n", + __func__, region.name, region.start, region.end - 1, addr, addr + write_len - 1); + + write_func_t *write_func = lookup_write_func_ptr(flash->chip); + int ret = write_func(flash, rbuf, addr, write_len); + if (ret) { + msg_gerr("%s: failed to write (%#08x..%#08x).\n", __func__, addr, addr + write_len - 1); + free(region.name); + return -1; + } + + free(region.name); + } + + return 0; } typedef int (probe_func_t)(struct flashctx *flash); -- To view, visit https://review.coreboot.org/c/flashrom/+/70516 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Idacf0d5218da9d9929f4877fc7665fe608b87fe0 Gerrit-Change-Number: 70516 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: flashrom: Check for flash access restricitons in verify_range()
by Nikolai Artemiev (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Edward O'Callaghan. Hello Edward O'Callaghan, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/70515 to review the following change. Change subject: flashrom: Check for flash access restricitons in verify_range() ...................................................................... flashrom: Check for flash access restricitons in verify_range() Make verify_flash() skip read/write-protected regions based on the FLASHROM_FLAG_SKIP_UNREADABLE and FLASHROM_FLAG_SKIP_UNWRITABLE flags. If FLASHROM_FLAG_SKIP_UNREADABLE is false, read-protected regions will cause verification to fail. If FLASHROM_FLAG_SKIP_UNWRITABLE is false, read-only regions will still be verified and any mismatch will cause verification to fail. It can be useful to set the flag to true so that expected mismatches in read-only regions are ignored by verify_range() after flashing. BUG=b:260440773 BRANCH=none TEST=flashrom -v on dedede (JSL) Change-Id: I61dfadd3c75365f2e55abeea75f673ab791ca5cc CoAuthored-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashrom.c 1 file changed, 68 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/15/70515/1 diff --git a/flashrom.c b/flashrom.c index 19068a7..76a2801 100644 --- a/flashrom.c +++ b/flashrom.c @@ -582,15 +582,49 @@ return -1; } - int ret = read_flash(flash, readbuf, start, len); - if (ret) { - msg_gerr("Verification impossible because read failed " - "at 0x%x (len 0x%x)\n", start, len); - ret = -1; - goto out_free; + int ret = 0; + + msg_gdbg("%#06x..%#06x ", start, start + len - 1); + + unsigned int read_len; + for (size_t addr = start; addr < start + len; addr += read_len) { + struct flash_region region; + get_flash_region(flash, addr, &region); + read_len = min(start + len, region.end) - addr; + + if ((region.write_prot && flash->flags.skip_unwritable_regions) || + (region.read_prot && flash->flags.skip_unreadable_regions)) { + msg_gdbg("%s: Skipping verification of %s region (%#08x..%#08x)\n", + __func__, region.name, region.start, region.end - 1); + free(region.name); + continue; + } + + if (region.read_prot) { + msg_gerr("%s: Verification imposible because %s region (%#08x..%#08x) is unreadable.\n", + __func__, region.name, region.start, region.end - 1); + free(region.name); + goto out_free; + } + + msg_gdbg("%s: Verifying %s region (%#08x..%#08x)\n", + __func__, region.name, region.start, region.end - 1); + free(region.name); + + ret = read_flash(flash, readbuf, addr, read_len); + if (ret) { + msg_gerr("Verification impossible because read failed " + "at 0x%x (len 0x%x)\n", start, len); + ret = -1; + goto out_free; + } + + ret = compare_range(cmpbuf + (addr - start), readbuf, addr, read_len); + if (ret) + goto out_free; + } - ret = compare_range(cmpbuf, readbuf, start, len); out_free: free(readbuf); return ret; -- To view, visit https://review.coreboot.org/c/flashrom/+/70515 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I61dfadd3c75365f2e55abeea75f673ab791ca5cc Gerrit-Change-Number: 70515 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: flashrom: Check for flash access restricitons in read_flash()
by Nikolai Artemiev (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Edward O'Callaghan. Hello Edward O'Callaghan, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/70514 to review the following change. Change subject: flashrom: Check for flash access restricitons in read_flash() ...................................................................... flashrom: Check for flash access restricitons in read_flash() Skip read-protected regions if FLASHROM_FLAG_SKIP_UNREADABLE_REGIONS is true. If the flag is false, read operations that include an read-protected region will return an error. BUG=b:260440773 BRANCH=none TEST=flashrom -r on dedede (JSL) Change-Id: I22c795d7d08ef8bf773733d9952967b2fa2ef299 CoAuthored-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M flashrom.c 1 file changed, 87 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/14/70514/1 diff --git a/flashrom.c b/flashrom.c index c26894b..19068a7 100644 --- a/flashrom.c +++ b/flashrom.c @@ -338,6 +338,21 @@ return extract_param(&cfg->params, param_name, ","); } +static void get_flash_region(const struct flashctx *flash, int addr, struct flash_region *region) +{ + if ((flash->mst->buses_supported & BUS_PROG) && flash->mst->opaque.get_region) { + flash->mst->opaque.get_region(flash, addr, region); + } else if (flash->mst->buses_supported & BUS_SPI && flash->mst->spi.get_region) { + flash->mst->spi.get_region(flash, addr, region); + } else { + region->name = strdup(""); + region->start = 0; + region->end = flashrom_flash_getsize(flash); + region->read_prot = false; + region->write_prot = false; + } +} + /* special unit-test hook */ erasefunc_t *g_test_erase_injector; @@ -486,10 +501,60 @@ return NULL; } +/* + * @brief Wrapper for flash->read() with additional high-level policy. + * + * @param flash flash chip + * @param buf buffer to store data in + * @param start start address + * @param len number of bytes to read + * @return 0 on success, + * -1 if any read fails. + * + * This wrapper simplifies most cases when the flash chip needs to be read + * since policy decisions such as non-fatal error handling is centralized. + */ int read_flash(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { - read_func_t *read_func = lookup_read_func_ptr(flash->chip); - return read_func(flash, buf, start, len); + unsigned int read_len; + for (unsigned int addr = start; addr < start + len; addr += read_len) { + struct flash_region region; + get_flash_region(flash, addr, &region); + + read_len = min(start + len, region.end) - addr; + uint8_t *rbuf = buf + addr - start; + + if (region.read_prot) { + if (flash->flags.skip_unwritable_regions) { + msg_gdbg("%s: cannot read inside %s region (%#08x..%#08x), " + "filling (%#08x..%#08x) with erased value instead.\n", + __func__, region.name, region.start, region.end - 1, + addr, addr + read_len - 1); + free(region.name); + + memset(rbuf, ERASED_VALUE(flash), read_len); + continue; + } + + msg_gerr("%s: cannot read inside %s region (%#08x..%#08x).\n", + __func__, region.name, region.start, region.end - 1); + free(region.name); + return -1; + } + msg_gdbg("%s: %s region (%#08x..%#08x) is readable, reading range (%#08x..%#08x).\n", + __func__, region.name, region.start, region.end - 1, addr, addr + read_len - 1); + + read_func_t *read_func = lookup_read_func_ptr(flash->chip); + int ret = read_func(flash, rbuf, addr, read_len); + if (ret) { + msg_gerr("%s: failed to read (%#08x..%#08x).\n", __func__, addr, addr + read_len - 1); + free(region.name); + return -1; + } + + } + + return 0; } /* -- To view, visit https://review.coreboot.org/c/flashrom/+/70514 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I22c795d7d08ef8bf773733d9952967b2fa2ef299 Gerrit-Change-Number: 70514 Gerrit-PatchSet: 1 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: libflashrom: Add flags to skup unreadable and unwritable regions
by Nikolai Artemiev (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev. Hello build bot (Jenkins), Edward O'Callaghan, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/70128 to look at the new patch set (#21). Change subject: libflashrom: Add flags to skup unreadable and unwritable regions ...................................................................... libflashrom: Add flags to skup unreadable and unwritable regions Add flags to allow libflashrom users to configure how operations that include unreadable or unwritable regions should be behave. If the flags are set to true, a read/write operation will just skip the inaccessible region and will still be executed in other regions. If the flags are set to false, the inaccessible region will cause the entire operation to fail. BUG=b:260440773 BRANCH=none TEST=builds Change-Id: I9b96fb04b863625d2c9f9a00b97c35b3ddb0871b CoAuthored-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> --- M include/flash.h M include/libflashrom.h M libflashrom.c 3 files changed, 42 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/28/70128/21 -- To view, visit https://review.coreboot.org/c/flashrom/+/70128 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9b96fb04b863625d2c9f9a00b97c35b3ddb0871b Gerrit-Change-Number: 70128 Gerrit-PatchSet: 21 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: tests: Add llvm-cov option and run target for code coverage
by Evan Benn (Code Review) 09 Dec '22

09 Dec '22

Attention is currently required from: Thomas Heijligen, 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/+/69268 to look at the new patch set (#18). Change subject: tests: Add llvm-cov option and run target for code coverage ...................................................................... tests: Add llvm-cov option and run target for code coverage Code coverage can be requested with -Dllvm_cov and run with ninja llvm-cov-tests or llvm-cov-cli. BUG=b:187647884 BRANCH=None TEST=meson test; ninja llvm-cov-tests TEST=ran test_build.sh with coverage enabled TEST=jenkins ran test_build.sh with coverage disabled Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M Documentation/building.md M meson.build M meson_options.txt A scripts/llvm-cov M tests/meson.build 5 files changed, 46 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/68/69268/18 -- 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: 18 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: tests: Detect llvm coverage run and redirect to real I/O functions
by Anastasia Klimchuk (Code Review) 09 Dec '22

09 Dec '22

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69267 ) ( 13 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )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> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69267 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M Documentation/building.md M tests/io_real.c M tests/tests.c M tests/wraps.h 4 files changed, 53 insertions(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/Documentation/building.md b/Documentation/building.md index 81daeb0..e25482e 100644 --- a/Documentation/building.md +++ b/Documentation/building.md @@ -60,6 +60,17 @@ ninja -C buildcov coverage ``` +#### llvm +https://clang.llvm.org/docs/SourceBasedCodeCoverage.html +``` +env CFLAGS="-fprofile-instr-generate -fcoverage-mapping" CC=clang meson setup buildclangcov +env LLVM_PROFILE_FILE=default.profraw ninja -C buildclangcov test +cd buildclangcov +llvm-profdata merge -sparse default.profraw -o default.profdata +llvm-cov show ./flashrom_unit_tests -instr-profile=default.profdata --format=html --output-dir=. +open index.html +``` + ## System specific information ### Ubuntu / Debian (Linux) * __linux-headers__ are version specific diff --git a/tests/io_real.c b/tests/io_real.c index 9f9e9d6..429027f 100644 --- a/tests/io_real.c +++ b/tests/io_real.c @@ -25,6 +25,12 @@ return __real_open(pathname, flags, mode); } +static FILE *io_real_fopen(void *state, const char *pathname, const char *mode) +{ + LOG_ME; + return __real_fopen(pathname, mode); +} + static FILE *io_real_fdopen(void *state, int fd, const char *mode) { LOG_ME; @@ -36,13 +42,21 @@ return __real_fwrite(ptr, size, nmemb, fp); } +static int io_real_fclose(void *state, FILE *fp) +{ + LOG_ME; + return __real_fclose(fp); +} + /* Mock ios that defer to the real io functions. * These exist so that code coverage can print to real files on disk. */ static const struct io_mock real_io = { .iom_open = io_real_open, + .iom_fopen = io_real_fopen, .iom_fwrite = io_real_fwrite, .iom_fdopen = io_real_fdopen, + .iom_fclose = io_real_fclose, }; /* Return 0 if string ends with suffix. */ @@ -58,6 +72,7 @@ void maybe_unmock_io(const char *pathname) { const char *gcov_suffix = ".gcda"; - if (!check_suffix(pathname, gcov_suffix)) + const char *llvm_cov_suffix = ".profraw"; + if (!check_suffix(pathname, gcov_suffix) || !check_suffix(pathname, llvm_cov_suffix)) io_mock_register(&real_io); } diff --git a/tests/tests.c b/tests/tests.c index b11f642..a1dcaca 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -170,6 +170,7 @@ FILE *__wrap_fopen(const char *pathname, const char *mode) { LOG_ME; + maybe_unmock_io(pathname); if (get_io() && get_io()->iom_fopen) return get_io()->iom_fopen(get_io()->state, pathname, mode); return not_null(); @@ -178,6 +179,7 @@ FILE *__wrap_fopen64(const char *pathname, const char *mode) { LOG_ME; + maybe_unmock_io(pathname); if (get_io() && get_io()->iom_fopen) return get_io()->iom_fopen(get_io()->state, pathname, mode); return not_null(); diff --git a/tests/wraps.h b/tests/wraps.h index 92b0c42..6cc9567 100644 --- a/tests/wraps.h +++ b/tests/wraps.h @@ -36,6 +36,7 @@ int __wrap_write(int fd, const void *buf, size_t sz); int __wrap_read(int fd, void *buf, size_t sz); FILE *__wrap_fopen(const char *pathname, const char *mode); +FILE *__real_fopen(const char *pathname, const char *mode); FILE *__wrap_fopen64(const char *pathname, const char *mode); FILE *__wrap_fdopen(int fd, const char *mode); FILE *__real_fdopen(int fd, const char *mode); @@ -59,6 +60,7 @@ int __wrap_fprintf(FILE *fp, const char *fmt, ...); int __wrap___vfprintf_chk(FILE *fp, const char *fmt, va_list args); int __wrap_fclose(FILE *fp); +int __real_fclose(FILE *fp); int __wrap_feof(FILE *fp); int __wrap_ferror(FILE *fp); void __wrap_clearerr(FILE *fp); -- 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: 15 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-MessageType: merged

1 0

[S] Change in flashrom[master]: tests: Detect gcov run and redirect to real I/O functions
by Anastasia Klimchuk (Code Review) 09 Dec '22

09 Dec '22

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69266 ) ( 13 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )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> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69266 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M Documentation/building.md M tests/tests.c 2 files changed, 35 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/Documentation/building.md b/Documentation/building.md index aabb45b..81daeb0 100644 --- a/Documentation/building.md +++ b/Documentation/building.md @@ -48,6 +48,18 @@ ninja -C builddir test ``` +### Run unit tests with code coverage +#### gcov +Due to a bug in lcov, the html file will only be correct if lcov is not +installed and gcovr is installed. See +https://github.com/linux-test-project/lcov/issues/168 +https://github.com/mesonbuild/meson/issues/6747 +``` +meson setup buildcov -Db_coverage=true +ninja -C buildcov test +ninja -C buildcov coverage +``` + ## System specific information ### Ubuntu / Debian (Linux) * __linux-headers__ are version specific diff --git a/tests/tests.c b/tests/tests.c index 41972ba..b11f642 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -17,6 +17,7 @@ #include "io_mock.h" #include "tests.h" #include "wraps.h" +#include "io_real.h" #include <stdio.h> #include <string.h> @@ -77,6 +78,7 @@ static int mock_open(const char *pathname, int flags, mode_t mode) { + maybe_unmock_io(pathname); if (get_io() && get_io()->iom_open) return get_io()->iom_open(get_io()->state, pathname, flags, mode); -- 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: 15 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-MessageType: merged

1 0

[M] Change in flashrom[master]: tests: Redirect to real I/O to support coverage run
by Anastasia Klimchuk (Code Review) 09 Dec '22

09 Dec '22

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69539 ) 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> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69539 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- A tests/io_real.c A tests/io_real.h M tests/meson.build M tests/wraps.h 4 files changed, 112 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/tests/io_real.c b/tests/io_real.c new file mode 100644 index 0000000..9f9e9d6 --- /dev/null +++ b/tests/io_real.c @@ -0,0 +1,63 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2022 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include "io_mock.h" +#include "wraps.h" + +#include "io_real.h" +#include <string.h> + +static int io_real_open(void *state, const char *pathname, int flags, mode_t mode) +{ + LOG_ME; + return __real_open(pathname, flags, mode); +} + +static FILE *io_real_fdopen(void *state, int fd, const char *mode) +{ + LOG_ME; + return __real_fdopen(fd, mode); +} + +static size_t io_real_fwrite(void *state, const void *ptr, size_t size, size_t nmemb, FILE *fp) +{ + return __real_fwrite(ptr, size, nmemb, fp); +} + +/* Mock ios that defer to the real io functions. + * These exist so that code coverage can print to real files on disk. + */ +static const struct io_mock real_io = { + .iom_open = io_real_open, + .iom_fwrite = io_real_fwrite, + .iom_fdopen = io_real_fdopen, +}; + +/* Return 0 if string ends with suffix. */ +static int check_suffix(const char *string, const char *suffix) +{ + int len_l = strlen(string); + int len_r = strlen(suffix); + if (len_l > len_r) + return strcmp(string + len_l - len_r, suffix); + return 1; +} + +void maybe_unmock_io(const char *pathname) +{ + const char *gcov_suffix = ".gcda"; + if (!check_suffix(pathname, gcov_suffix)) + io_mock_register(&real_io); +} diff --git a/tests/io_real.h b/tests/io_real.h new file mode 100644 index 0000000..f2491c3 --- /dev/null +++ b/tests/io_real.h @@ -0,0 +1,24 @@ +/* + * This file is part of the flashrom project. + * + * Copyright 2022 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef IO_REAL_H +#define IO_REAL_H + +/* Detect file io that should not be mocked, for example code coverage writing + * gcda files. Call io_mock_register with functions that defer to real io. + */ +void maybe_unmock_io(const char* pathname); + +#endif /* IO_REAL_H */ diff --git a/tests/meson.build b/tests/meson.build index b8a5ba8..0d605d2 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -23,6 +23,7 @@ 'chip.c', 'chip_wp.c', 'selfcheck.c', + 'io_real.c', ) if not programmer.get('dummy').get('active') diff --git a/tests/wraps.h b/tests/wraps.h index e224d79..92b0c42 100644 --- a/tests/wraps.h +++ b/tests/wraps.h @@ -29,6 +29,7 @@ void __wrap_sio_write(uint16_t port, uint8_t reg, uint8_t data); uint8_t __wrap_sio_read(uint16_t port, uint8_t reg); int __wrap_open(const char *pathname, int flags, ...); +int __real_open(const char *pathname, int flags, ...); int __wrap_open64(const char *pathname, int flags, ...); int __wrap___open64_2(const char *pathname, int flags, ...); int __wrap_ioctl(int fd, unsigned long int request, ...); @@ -37,6 +38,7 @@ FILE *__wrap_fopen(const char *pathname, const char *mode); FILE *__wrap_fopen64(const char *pathname, const char *mode); FILE *__wrap_fdopen(int fd, const char *mode); +FILE *__real_fdopen(int fd, const char *mode); int __wrap_stat(const char *path, void *buf); int __wrap_stat64(const char *path, void *buf); int __wrap___xstat(const char *path, void *buf); @@ -49,6 +51,7 @@ char *__wrap___fgets_chk(char *buf, int len, FILE *fp); size_t __wrap_fread(void *ptr, size_t size, size_t nmemb, FILE *fp); size_t __wrap_fwrite(const void *ptr, size_t size, size_t nmemb, FILE *fp); +size_t __real_fwrite(const void *ptr, size_t size, size_t nmemb, FILE *fp); int __wrap_fflush(FILE *fp); int __wrap_fileno(FILE *fp); int __wrap_fsync(int fd); -- 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: 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-MessageType: merged

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit