https://github.com/rust-lang/rust http://git.dreamy.place/mirrors/rust/atom?h=try 2020-08-30T15:45:07+00:00 2020-08-30T15:45:07+00:00 mark markm@cs.wisc.edu 2020-08-28T03:58:48+00:00 urn:sha1:9e5f7d5631b8f4009ac1c693e585d4b7108d4275 2020-08-19T04:06:45+00:00 Rich Kadel richkadel@google.com 2020-08-15T11:42:13+00:00 urn:sha1:d129ac2655451ca894bceeb58a4d5c26a86c4caa 2020-08-15T03:29:12+00:00 pawanbisht62 36775517+pawanbisht62@users.noreply.github.com 2020-08-15T03:29:12+00:00 urn:sha1:bab15885c0ab1230186cc45290c6d6c224289f6b 2020-08-14T16:27:00+00:00 bors bors@rust-lang.org 2020-08-14T16:27:00+00:00 urn:sha1:8e21bd0633b8d970646ee6eb706c9e8acfad19af LLVM IR coverage encoding aligns closer to Clang's I found some areas for improvement while attempting to debug the SegFault issue when running rust programs compiled using MSVC, with coverage instrumentation. I discovered that LLVM's coverage writer was generating incomplete function name variable names (that's not a typo: the name of the variable that holds a function name). The existing implementation used one-up numbers to distinguish variables, and correcting the names did not fix the MSVC coverage bug, but the fix in this PR makes the names and resulting LLVM IR easier to follow and more consistent with Clang's implementation. I also changed the way the `-Zinstrument-coverage` option is supported in symbol_export.rs. The original implementation was incorrect, and the corrected version matches the handling for `-Zprofile-generate`, as it turns out. (An argument could be made that maybe `-Zinstrument-coverage` should automatically enable `-Cprofile-generate`. In fact, if `-Cprofile-generate` is analagous to Clang's `-fprofile-generate`, as some documentation implies, Clang always requires this flag for its implementation of source-based code coverage. This would require a little more validation, and if implemented, would probably require updating some of the user-facing messages related to `-Cprofile-generate` to not be so specific to the PGO use case.) None of these changes fixed the MSVC coverage problems, but they should still be welcome improvements. Lastly, I added some additional FIXME comments in instrument_coverage.rs describing issues I found with the generated LLVM IR that would be resolved if the coverage instrumentation is injected with a `Statement` instead of as a new `BasicBlock`. I describe seven advantages of this change, but it requires some discussion before making a change like this. r? @tmandry 2020-08-14T09:24:35+00:00 Rich Kadel richkadel@google.com 2020-08-06T05:53:11+00:00 urn:sha1:ba189788758a68a921b349e07109c18a51f18bda I found some areas for improvement while attempting to debug the SegFault issue when running rust programs compiled using MSVC, with coverage instrumentation. I discovered that LLVM's coverage writer was generating incomplete function name variable names (that's not a typo: the name of the variable that holds a function name). The existing implementation used one-up numbers to distinguish variables, and correcting the names did not fix the MSVC coverage bug, but the fix in this PR makes the names and resulting LLVM IR easier to follow and more consistent with Clang's implementation. I also changed the way the `-Zinstrument-coverage` option is supported in symbol_export.rs. The original implementation was incorrect, and the corrected version matches the handling for `-Zprofile-generate`, as it turns out. (An argument could be made that maybe `-Zinstrument-coverage` should automatically enable `-Cprofile-generate`. In fact, if `-Cprofile-generate` is analagous to Clang's `-fprofile-generate`, as some documentation implies, Clang always requires this flag for its implementation of source-based code coverage. This would require a little more validation, and if implemented, would probably require updating some of the user-facing messages related to `-Cprofile-generate` to not be so specific to the PGO use case.) None of these changes fixed the MSVC coverage problems, but they should still be welcome improvements. Lastly, I added some additional FIXME comments in instrument_coverage.rs describing issues I found with the generated LLVM IR that would be resolved if the coverage instrumentation is injected with a `Statement` instead of as a new `BasicBlock`. I describe seven advantages of this change, but it requires some discussion before making a change like this. 2020-08-10T07:49:24+00:00 pawanbisht62 36775517+pawanbisht62@users.noreply.github.com 2020-08-10T07:49:24+00:00 urn:sha1:7c4ef37e19c0f9f501c04dd7bb1d5b864f0b6a2b 2020-08-09T19:25:21+00:00 Josh Stone jistone@redhat.com 2020-08-08T03:42:29+00:00 urn:sha1:1f71f0f2b54981d4cc7a99201d180e787c13ca6d 2020-08-06T11:30:51+00:00 pawanbisht62 36775517+pawanbisht62@users.noreply.github.com 2020-08-06T11:30:51+00:00 urn:sha1:bac939edcf2137f625cfd7b56dfa4aa7b814868f 2020-08-06T11:26:56+00:00 bishtpawan pawan.bisht@knoldus.com 2020-08-05T11:35:53+00:00 urn:sha1:fdfbd89946ca34d12eec658d111ce9a85cd23df0 2020-08-04T18:06:54+00:00 Rich Kadel richkadel@google.com 2020-08-02T03:03:59+00:00 urn:sha1:e0dc8dec273b4cba44a91c1b4433e3dcd117919f The prior PR corrected for errors encountered when trying to generate the coverage map on source code inlined from external crates (including macros and generics) by avoiding adding external DefIds to the coverage map. This made it possible to generate a coverage report including external crates, but the external crate coverage was incomplete (did not include coverage for the DefIds that were eliminated. The root issue was that the coverage map was converting Span locations to source file and locations, using the SourceMap for the current crate, and this would not work for spans from external crates (compliled with a different SourceMap). The solution was to convert the Spans to filename and location during MIR generation instead, so precompiled external crates would already have the correct source code locations embedded in their MIR, when imported into another crate.