Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reports: Fix a possible infinite loop in SourceManager #748

Merged
merged 1 commit into from
Sep 16, 2020

Conversation

meklort
Copy link
Contributor

@meklort meklort commented Sep 14, 2020

fgetc returns an int, however char was specified.
This fixes a case where the EOF is truncated to a char
which may not be detected depending on the sign of a char.

This issue is known to trigger on ppc64le.

fgetc returns an int, however char was specified.
This fixes a case where the EOF is truncated to a char
which may not be detected depending on the sign of a char.

This issue is known to trigger on ppc64le.
@AlexDenisov
Copy link
Member

Thanks, @meklort. Other than this and #751, does it work for you on the ppc64le?

@AlexDenisov AlexDenisov merged commit d090b68 into mull-project:master Sep 16, 2020
@meklort meklort deleted the inf-loop branch September 16, 2020 12:45
@meklort
Copy link
Contributor Author

meklort commented Sep 16, 2020

@AlexDenisov regarding ppc64le:

I have a couple of other changes, but I don't think they are issues only related to ppc64le.
meklort@bb8e087 disables crash dumping and speeds up mull tests when a test mutation causes a crash. I don't think this is the right place to add the code, but it was a quick change that got thing working better for me (64 threads = a big hit on the file system otherwise). There's a very noticeable speedup for me with this.

I also have a set of changes to get things working on LLVM 10.0.1, although I expect those break compat with older version of LLVM.
https://github.com/meklort/mull/tree/llvm-10.x.x (I noted after doing this taht you already have an llvm-10 branch)

Finally, on Fedora 33, I ran into a number of issues with functions not returning properly due to some return paths not being covered This is a quick change to get it to build, however I'm sure you might want something else.:
meklort@132beec

Do note that I am still getting a crash when I try to use the elements reporter. I have not yet looked into this. (I'll probably test with llvm 9 first)

@jnohlgard
Copy link
Contributor

@meklort Re: Crash in elements reporter
I have noticed that I get a failed assertion in the Elements reporter around missing entries in some map if I run mull-cxx without specifying --compdb-path=build/compile_commands.json. I have not had time to try to fix it, but specifying a compile_commands.json file seems to avoid the crash.

@AlexDenisov
Copy link
Member

I have a couple of other changes, but I don't think they are issues only related to ppc64le.
meklort/mull@bb8e087 disables crash dumping and speeds up mull tests when a test mutation causes a crash. I don't think this is the right place to add the code, but it was a quick change that got thing working better for me (64 threads = a big hit on the file system otherwise). There's a very noticeable speedup for me with this.

This patch would be greatly appreciated! I think having it in main is not a problem, I'd probably hide it behind a flag with disabled core dumps by default with something like -enable-core-dumps in case we ever want to turn them on.

I also have a set of changes to get things working on LLVM 10.0.1, although I expect those break compat with older version of LLVM.
https://github.com/meklort/mull/tree/llvm-10.x.x (I noted after doing this taht you already have an llvm-10 branch)

There is a compatibility layer for different versions of LLVM. There is a branch indeed, but I didn't finish the work back then and was planning to get it done soon. I'm happy to take care of it unless you want to bring it on yourself :)

Finally, on Fedora 33, I ran into a number of issues with functions not returning properly due to some return paths not being covered This is a quick change to get it to build, however I'm sure you might want something else.:
meklort/mull@132beec

I guess this is because of gcc? We don't explicitly support gcc assuming that if anyone wants to use an LLVM based tool, then they should have access to clang :)
Anyhow, your commit looks good to me so do not hesitate to open another PR.

Do note that I am still getting a crash when I try to use the elements reporter. I have not yet looked into this. (I'll probably test with llvm 9 first)

That is certainly a bug. I'd suggest you give a try to @gebart's suggestion, but we should certainly do better and issue a warning and not crash in this case.

@meklort
Copy link
Contributor Author

meklort commented Sep 18, 2020

This patch would be greatly appreciated! I think having it in main is not a problem, I'd probably hide it behind a flag with disabled core dumps by default with something like -enable-core-dumps in case we ever want to turn them on.

Sure thing, I'll try send something your way this weekend.

There is a compatibility layer for different versions of LLVM. There is a branch indeed, but I didn't finish the work back then and was planning to get it done soon. I'm happy to take care of it unless you want to bring it on yourself :)

Everything is working on my branch, so I'll also try to send it your way later.

I guess this is because of gcc? We don't explicitly support gcc assuming that if anyone wants to use an LLVM based tool, then they should have access to clang :)
Anyhow, your commit looks good to me so do not hesitate to open another PR.

Will do. It looks like /usr/bin/c++ defaults to GCC 10 on Fedora, and so cmake will use this by default. I was running the following;
cmake .. -G Ninja -DPATH_TO_LLVM=/llvm-bcm5719/ -DCMAKE_INSTALL_PREFIX=/llvm-bcm5719
(I didn't see any build guide stating to set CC/CXX to clang/clang++)
Note:
/usr/bin/c++ --version
c++ (GCC) 10.2.1 20200723 (Red Hat 10.2.1-1)

I'll do a pull request in a few minutes for it.

Do note that I am still getting a crash when I try to use the elements reporter. I have not yet looked into this. (I'll probably test with llvm 9 first)

That is certainly a bug. I'd suggest you give a try to @gebart's suggestion, but we should certainly do better and issue a warning and not crash in this case.

That certainly changes the behvaiour (no mutants remain - all are filtered out) (no more assertion), however all mutants are skipped. I'll take a look at it more when I get some free time. (Note that the IDE reporter works fine).

Also note that I'm getting some intermittent behaviour where run-to-run I get different results (anywhere from 87% up to 100% killed). I personally would expect that to be consistent (test isn't randomized). So, I'll also need to look into that more (try LLVM9, x86, etc).

@AlexDenisov
Copy link
Member

(I didn't see any build guide stating to set CC/CXX to clang/clang++)

Makes sense, I'll add it as a recommendation.

That certainly changes the behvaiour (no mutants remain - all are filtered out) (no more assertion), however all mutants are skipped. I'll take a look at it more when I get some free time. (Note that the IDE reporter works fine).

Hm-hm, how many mutants do you have before filtering?

Also note that I'm getting some intermittent behaviour where run-to-run I get different results (anywhere from 87% up to 100% killed). I personally would expect that to be consistent (test isn't randomized). So, I'll also need to look into that more (try LLVM9, x86, etc).

Some little discrepancy is possible, but 87-100 looks like something is off.
One of the reasons the results may differ is crashes - let's say a mutant writes out of bounds, then on some runs it will crash, while on the others it won't.
The other reason is multithreading - at this very moment, Mull will fail to provide consistent and valid results if the program under test runs several tests. I think this is not mentioned anywhere in the docs. This needs to be fixed anyways, I just didn't get to it yet.

@AlexDenisov
Copy link
Member

87-100 looks like something is off.

Assuming the number of mutants if big enough for this difference to be significant.

@meklort
Copy link
Contributor Author

meklort commented Sep 18, 2020

87-100 looks like something is off.

Assuming the number of mutants if big enough for this difference to be significant.

Good point, this was a single test case w/ 33 mutants total, so 29/33 mutants on the low end, 33/33 on the high end. This is a small enough sample right now that it's probably not a problem. That said... the variability is a bit concerning, although I do understand the explanation.

Note that I have implemented mutation testing in an instruction set simulator (assembly/opcode level mutations) and in that system no MMU exists, so crashing wasn't a valid kill case. In this case, I'm rebuilding firmware from an embedded system (with no MMU).

For the code base I'm working on now, I'm planning on adding a gate based on the mutation score so that it never decreases before merge, however that will get a bit tricky with the variability.

@meklort
Copy link
Contributor Author

meklort commented Sep 18, 2020

Hm-hm, how many mutants do you have before filtering?

I'm filtering out mutants in the test code and system libraries with --exclude-path, and with that I get down to 33 from ~500. Note that as soon as I enable compile_commands.json (even without --exclude-path), none are tested.

Note that it looks like compile_commands.json may be causing problems in my case as files are build multiple times to different targets (ARM, MIPS, and host) depending on where they are linking (different firmware binaries or host simulator). I did try manually removing the arm variant from the file, but that didn't seem to help. It may also be that I'm passing the files to clang and not clang++.

{
"directory": "/home/meklort/git/bcm5719 fw/build",
"command": "/home/meklort/llvm-bcm5719/bin/clang -DVERSION_MAJOR=0 -DVERSION_MINOR=4 -DVERSION_PATCH=382 -I../libs/NCSI/../../include -I../libs/NCSI/include -I../libs/MII/../../include -I../libs/MII/include -I../libs/APE/../../include -I../libs/APE/include -I../libs/printf/../../include -I../libs/printf/. -I../libs/Network/../../include -I../libs/Network/include -Werror -Wall -O3 -nostdlib -nodefaultlibs -ffunction-sections -fdata-sections -fomit-frame-pointer -fno-builtin -target thumbv7-none-eabi -mcpu=cortex-m3 -mfloat-abi=soft -o libs/NCSI/CMakeFiles/NCSI-arm.dir/ncsi.c.o -c "/home/meklort/git/bcm5719 fw/libs/NCSI/ncsi.c"",
"file": "/home/meklort/git/bcm5719 fw/libs/NCSI/ncsi.c"
},

{
"directory": "/home/meklort/git/bcm5719 fw/build",
"command": "/home/meklort/llvm-bcm5719/bin/clang -DVERSION_MAJOR=0 -DVERSION_MINOR=4 -DVERSION_PATCH=382 -I../libs/NCSI/../../include -I../libs/NCSI/include -I../simulator/include -I../simulator/../include -I../libs/MII/../../include -I../libs/MII/include -I../libs/APE/../../include -I../libs/APE/include -I../libs/Network/../../include -I../libs/Network/include -Werror -Wall -O3 -DCXX_SIMULATOR -x c++ -fembed-bitcode -g -Wno-c99-designator -Wno-reorder-init-list -o libs/NCSI/CMakeFiles/NCSI.dir/ncsi.c.o -c "/home/meklort/git/bcm5719 fw/libs/NCSI/ncsi.c"",
"file": "/home/meklort/git/bcm5719 fw/libs/NCSI/ncsi.c"
},
`

One of the reasons the results may differ is crashes - let's say a mutant writes out of bounds, then on some runs it will crash, while on the others it won't.
Crashes make sense here. I'm thinking I may want to disable that as a valid way to kill mutants at some point..

The other reason is multithreading - at this very moment, Mull will fail to provide consistent and valid results if the program under test runs several tests. I think this is not mentioned anywhere in the docs. This needs to be fixed anyways, I just didn't get to it yet.
This may be it. My system has 96 threads and is triggering a google-test runner. That said, right now there's only one test in the test binary.

@AlexDenisov
Copy link
Member

Good point, this was a single test case w/ 33 mutants total, so 29/33 mutants on the low end, 33/33 on the high end. This is a small enough sample right now that it's probably not a problem. That said... the variability is a bit concerning, although I do understand the explanation.

Is it a private or public project? I would be happy to take a closer look at the results if possible, it could still be that there is a bug somewhere.

I'm filtering out mutants in the test code and system libraries with --exclude-path, and with that I get down to 33 from ~500. Note that as soon as I enable compile_commands.json (even without --exclude-path), none are tested.

But does the rest (33) looks like the correct mutants? The primary goal of this filtering is to remove the mutants that exist on the Bitcode level, but do not have a correspomding representation on the source code level. E.g., some branches and method calls generate from exceptions or macros/compiler builtins.

Note that it looks like compile_commands.json may be causing problems in my case as files are build multiple times to different targets (ARM, MIPS, and host) depending on where they are linking (different firmware binaries or host simulator). I did try manually removing the arm variant from the file, but that didn't seem to help. It may also be that I'm passing the files to clang and not clang++.

We certainly were not prepared for that. In this case, Mull will pick one of the commands for a file, but which one is undefined
I'll think about it, not yet sure how to handle this case gracefully on our side.

@meklort
Copy link
Contributor Author

meklort commented Sep 19, 2020

Is it a private or public project? I would be happy to take a closer look at the results if possible, it could still be that there is a bug somewhere.

Public: https://github.com/meklort/bcm5719-fw/tree/mutation-testing
Note that right now you'll need to manually add -fembed-bitcode to cmake/simulator.cmake. I'll work on pushing the changes I have within a day or two.
Note that you'll also need to build a modified version of LLVM for the code as-is, or else you'll need to modify the build system.

But does the rest (33) looks like the correct mutants? The primary goal of this filtering is to remove the mutants that exist on the Bitcode level, but do not have a correspomding representation on the source code level. E.g., some branches and method calls generate from exceptions or macros/compiler builtins.

Yes, the rest look reasonable. The only ones that are potentially questionable are mutations on a switch statement, however they still look useful there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants