Differential D97462
[clang][cli] Round-trip cc1 arguments in assert builds Authored by jansvoboda11 on Feb 25 2021, 4:27 AM.
Details This patch enables cc1 argument round-trip for assert builds. It can be disabled by building clang with -DCLANG_ROUND_TRIP_CC1_ARGS=OFF. This will be committed only if we reach consensus in https://lists.llvm.org/pipermail/cfe-dev/2021-February/067714.html.
Diff Detail
Event TimelineComment Actions How expensive are these checks? If it is non-trivial overhead, maybe it should default to ${LLVM_ENABLE_EXPENSIVE_CHECKS} instead? Comment Actions Thanks for bringing that up. I measured the performance overhead of round-tripping here: https://reviews.llvm.org/D95516. Command-line parsing itself is ~3x slower, but still takes under .5 ms. Compared to other parts of the compiler, this is insignificant. Changing the default to LLVM_ENABLE_EXPENSIVE_CHECKS would shift the check from local development to post-commit testing on built bots for most people. I don't think that would be a great trade-off given the negligible performance impact. Comment Actions In Chrome we noticed that plugin flags are not being roundtripped (and build fails with error: Generated arguments do not match in round-trip): example of the differing args: "-plugin-arg-blink-gc-plugin" "no-members-in-stack-allocated" "-plugin-arg-find-bad-constructs" "checked-ptr-as-trivial-member" "-plugin-arg-find-bad-constructs" "check-ipc" vs "-plugin-arg-find-bad-constructs" "checked-ptr-as-trivial-member" "-plugin-arg-find-bad-constructs" "check-ipc" "-plugin-arg-blink-gc-plugin" "no-members-in-stack-allocated" Comment Actions Thanks for reporting that. D99606 fixes one aspect of -plugin-arg, but it seems the order of generation is non-deterministic (most likely related to the underlying storage, std::unordered_map). I can look into it early next week, but I think simple sort in the generation code should do the trick. Comment Actions I just merged this commit into our CHERI fork and noticed some failing tests due to round tripping: For example, I added an additional std::string CHERIStatsFile; to CodeGenOptions. This is set inside bool CompilerInvocation::ParseCodeGenArgs using Opts.CHERIStatsFile = Args.getLastArgValue(OPT_cheri_stats_file).str();. Unfortunately this is not caught by any assertions, so I worry that there are other arguments that might be silently removed after this commit. Are there any open reviews/plans to check CodeGenOptions/etc, for equality after round-tripping? Comment Actions We currently don't have the infrastructure to compare CompilerInvocation instances directly. Instead we rely on good test coverage of command line options: if the round-tripped CompilerInvocation doesn't contain the option, tests will fail. You can then check the generated command lines by passing -Rround-trip-cc1-args to the failing CC1 invocation. There was an attempt to generate operator== for CompilerInvocation and assert when the round-tripped instance isn't equal to the original, as you suggest. This is the patch that started by moving members to definition databases, but it got reverted: D86290. Comment Actions Thanks, that would have caught the problem. Fortunately we don't add too many options, so I'll just go through them all and check that they are also handled in the Generate* functions. |