[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added a comment.

In D97462#2678889 , @jansvoboda11 
wrote:

> In D97462#2677130 , @arichardson 
> wrote:
>
>> I just merged this commit into our CHERI fork and noticed some failing tests 
>> due to round tripping:
>> We add some additional CodeGenOptions and LangOptions, but are not including 
>> those in the generated command line.
>>
>> 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();`.
>> I haven't added logic to round trip this flag (yet). If CC1 argument round 
>> tripping is enabled, the flag is stripped and the output goes to stderr 
>> instead of the defined file, causing some tests to fail.
>>
>> 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?
>
> 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 .

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D97462#2677130 , @arichardson wrote:

> I just merged this commit into our CHERI fork and noticed some failing tests 
> due to round tripping:
> We add some additional CodeGenOptions and LangOptions, but are not including 
> those in the generated command line.
>
> 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();`.
> I haven't added logic to round trip this flag (yet). If CC1 argument round 
> tripping is enabled, the flag is stripped and the output goes to stderr 
> instead of the defined file, causing some tests to fail.
>
> 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?

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 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added a comment.

I just merged this commit into our CHERI fork and noticed some failing tests 
due to round tripping:
We add some additional CodeGenOptions and LangOptions, but are not including 
those in the generated command line.

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();`.
I haven't added logic to round trip this flag (yet). If CC1 argument round 
tripping is enabled, the flag is stripped and the output goes to stderr instead 
of the defined file, causing some tests to fail.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D97462#298 , @dexonsmith wrote:

> In D97462#2666485 , @jansvoboda11 
> wrote:
>
>> 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.
>
> Can/should it just be changed to a `std::map`?

Yes, that that works as well. Fixed in D99879 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

In D97462#2666485 , @jansvoboda11 
wrote:

> 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.

Can/should it just be changed to a `std::map`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D97462#2666366 , @akhuang wrote:

> 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"

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Amy Huang via Phabricator via cfe-commits]

akhuang added a comment.

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"


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Jan Svoboda via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbb88a5aeee68: [clang][cli] Round-trip cc1 arguments in 
assert builds (authored by jansvoboda11).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

Files:
  clang/CMakeLists.txt


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -467,7 +467,8 @@
 
 option(CLANG_ENABLE_PROTO_FUZZER "Build Clang protobuf fuzzer." OFF)
 
-option(CLANG_ROUND_TRIP_CC1_ARGS "Round-trip command line arguments in -cc1." 
OFF)
+option(CLANG_ROUND_TRIP_CC1_ARGS
+  "Round-trip command line arguments in -cc1." ${LLVM_ENABLE_ASSERTIONS})
 
 if(NOT CLANG_ENABLE_STATIC_ANALYZER AND CLANG_ENABLE_ARCMT)
   message(FATAL_ERROR "Cannot disable static analyzer while enabling ARCMT or 
Z3")


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -467,7 +467,8 @@
 
 option(CLANG_ENABLE_PROTO_FUZZER "Build Clang protobuf fuzzer." OFF)
 
-option(CLANG_ROUND_TRIP_CC1_ARGS "Round-trip command line arguments in -cc1." OFF)
+option(CLANG_ROUND_TRIP_CC1_ARGS
+  "Round-trip command line arguments in -cc1." ${LLVM_ENABLE_ASSERTIONS})
 
 if(NOT CLANG_ENABLE_STATIC_ANALYZER AND CLANG_ENABLE_ARCMT)
   message(FATAL_ERROR "Cannot disable static analyzer while enabling ARCMT or Z3")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 added a comment.

In D97462#2644406 , @arichardson wrote:

> How expensive are these checks? If it is non-trivial overhead, maybe it 
> should default to `${LLVM_ENABLE_EXPENSIVE_CHECKS}` instead?

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Alexander Richardson via Phabricator via cfe-commits]

arichardson added a comment.

How expensive are these checks? If it is non-trivial overhead, maybe it should 
default to `${LLVM_ENABLE_EXPENSIVE_CHECKS}` instead?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

LGTM!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 328460.
jansvoboda11 added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

Files:
  clang/CMakeLists.txt


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -465,7 +465,8 @@
 
 option(CLANG_ENABLE_PROTO_FUZZER "Build Clang protobuf fuzzer." OFF)
 
-option(CLANG_ROUND_TRIP_CC1_ARGS "Round-trip command line arguments in -cc1." 
OFF)
+option(CLANG_ROUND_TRIP_CC1_ARGS
+  "Round-trip command line arguments in -cc1." ${LLVM_ENABLE_ASSERTIONS})
 
 if(NOT CLANG_ENABLE_STATIC_ANALYZER AND CLANG_ENABLE_ARCMT)
   message(FATAL_ERROR "Cannot disable static analyzer while enabling ARCMT or 
Z3")


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -465,7 +465,8 @@
 
 option(CLANG_ENABLE_PROTO_FUZZER "Build Clang protobuf fuzzer." OFF)
 
-option(CLANG_ROUND_TRIP_CC1_ARGS "Round-trip command line arguments in -cc1." OFF)
+option(CLANG_ROUND_TRIP_CC1_ARGS
+  "Round-trip command line arguments in -cc1." ${LLVM_ENABLE_ASSERTIONS})
 
 if(NOT CLANG_ENABLE_STATIC_ANALYZER AND CLANG_ENABLE_ARCMT)
   message(FATAL_ERROR "Cannot disable static analyzer while enabling ARCMT or Z3")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 326677.
jansvoboda11 added a comment.

Rebase on top of D97552 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

Files:
  clang/CMakeLists.txt


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -465,7 +465,8 @@
 
 option(CLANG_ENABLE_PROTO_FUZZER "Build Clang protobuf fuzzer." OFF)
 
-option(CLANG_ROUND_TRIP_CC1_ARGS "Round-trip command line arguments in -cc1." 
OFF)
+option(CLANG_ROUND_TRIP_CC1_ARGS
+  "Round-trip command line arguments in -cc1." ${LLVM_ENABLE_ASSERTIONS})
 
 if(NOT CLANG_ENABLE_STATIC_ANALYZER AND CLANG_ENABLE_ARCMT)
   message(FATAL_ERROR "Cannot disable static analyzer while enabling ARCMT or 
Z3")


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -465,7 +465,8 @@
 
 option(CLANG_ENABLE_PROTO_FUZZER "Build Clang protobuf fuzzer." OFF)
 
-option(CLANG_ROUND_TRIP_CC1_ARGS "Round-trip command line arguments in -cc1." OFF)
+option(CLANG_ROUND_TRIP_CC1_ARGS
+  "Round-trip command line arguments in -cc1." ${LLVM_ENABLE_ASSERTIONS})
 
 if(NOT CLANG_ENABLE_STATIC_ANALYZER AND CLANG_ENABLE_ARCMT)
   message(FATAL_ERROR "Cannot disable static analyzer while enabling ARCMT or Z3")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 updated this revision to Diff 326398.
jansvoboda11 added a comment.

Rebase on top of D97461 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97462/new/

https://reviews.llvm.org/D97462

Files:
  clang/CMakeLists.txt


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -465,7 +465,8 @@
 
 option(CLANG_ENABLE_PROTO_FUZZER "Build Clang protobuf fuzzer." OFF)
 
-option(CLANG_ROUND_TRIP_CC1_ARGS "Round-trip command line arguments in -cc1." 
OFF)
+option(CLANG_ROUND_TRIP_CC1_ARGS
+  "Round-trip command line arguments in -cc1." ${LLVM_ENABLE_ASSERTIONS})
 
 if(NOT CLANG_ENABLE_STATIC_ANALYZER AND CLANG_ENABLE_ARCMT)
   message(FATAL_ERROR "Cannot disable static analyzer while enabling ARCMT or 
Z3")


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -465,7 +465,8 @@
 
 option(CLANG_ENABLE_PROTO_FUZZER "Build Clang protobuf fuzzer." OFF)
 
-option(CLANG_ROUND_TRIP_CC1_ARGS "Round-trip command line arguments in -cc1." OFF)
+option(CLANG_ROUND_TRIP_CC1_ARGS
+  "Round-trip command line arguments in -cc1." ${LLVM_ENABLE_ASSERTIONS})
 
 if(NOT CLANG_ENABLE_STATIC_ANALYZER AND CLANG_ENABLE_ARCMT)
   message(FATAL_ERROR "Cannot disable static analyzer while enabling ARCMT or Z3")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97462: [clang][cli] Round-trip cc1 arguments in assert builds

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 created this revision.
jansvoboda11 added reviewers: Bigcheese, dexonsmith.
Herald added a subscriber: mgorny.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D97462

Files:
  clang/CMakeLists.txt


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -465,7 +465,8 @@
 
 option(CLANG_ENABLE_PROTO_FUZZER "Build Clang protobuf fuzzer." OFF)
 
-option(CLANG_ROUND_TRIP_CC1_ARGS "Round-trip command line arguments in -cc1." 
OFF)
+option(CLANG_ROUND_TRIP_CC1_ARGS
+  "Round-trip command line arguments in -cc1." ${LLVM_ENABLE_ASSERTIONS})
 
 if(NOT CLANG_ENABLE_STATIC_ANALYZER AND CLANG_ENABLE_ARCMT)
   message(FATAL_ERROR "Cannot disable static analyzer while enabling ARCMT or 
Z3")


Index: clang/CMakeLists.txt
===
--- clang/CMakeLists.txt
+++ clang/CMakeLists.txt
@@ -465,7 +465,8 @@
 
 option(CLANG_ENABLE_PROTO_FUZZER "Build Clang protobuf fuzzer." OFF)
 
-option(CLANG_ROUND_TRIP_CC1_ARGS "Round-trip command line arguments in -cc1." OFF)
+option(CLANG_ROUND_TRIP_CC1_ARGS
+  "Round-trip command line arguments in -cc1." ${LLVM_ENABLE_ASSERTIONS})
 
 if(NOT CLANG_ENABLE_STATIC_ANALYZER AND CLANG_ENABLE_ARCMT)
   message(FATAL_ERROR "Cannot disable static analyzer while enabling ARCMT or Z3")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits