-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Remove flags that may escalate warnings to errors #12116
Conversation
/gcbrun |
if strings.HasPrefix(arg, "-Werror") || | ||
arg == "-pedantic-errors" || | ||
arg == "-Wfatal-errors" || | ||
arg == "-Weverything" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be just -Werror
and -Wfatal-errors
instead? The other two (at least -Weverything
) should not result in a compilation error when they trigger, afaik
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Mihai!
Yes, I should remove -Weverything
. I was under the (incorrect) impression that too many warnings would somehow cause the command to fail.
I add -pedantic-errors
here because I reckon it will generate errors for any code that does not strictly adhere to the ISO C or ISO C++ standards.
Please let me know if I misinterpreted it : )
if retcode == 0 { | ||
WriteStdErrOut(fullCmdArgs, out, errstr) | ||
os.Exit(0) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines are identical to the original except for their indentations.
The original lines are misaligned with others, likely due to space/tab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thank you.
Need to experiment with this to see if the way Bazel detects compiler features is impacted (it compiles a default .cc
file for each feature under test, it can test for example if -Werror
is supported by looking at what gets compiled when the flag is passed). Also, one extra compilation might increase number of times we see the same error message, so confuse the error parser, the prompter fixer.
But for now, let's go with this, if nothing breaks we're good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Thanks! I conducted an experiment with C projects last night, here is the result: I will look into them today and pay extra attention to Bazel projects. |
Observing many compatibility issues caused by building a C++ fuzz target with C projects, I want to test whether separating them can lead to a higher compilation rate. This PR experiment uses C-specific prompts and examples for C projects. It also uses the fix in google/oss-fuzz#12116
Some projects failed to compile because its compilation command escalates warnings into errors (e.g.,
bind9
). This is particularly problematic for C projects becauseJCC
assumes these errors are due to C/C++ compatibility issues and then compiles these projects withclang++
, which causes different failures that LLM cannot fix (e.g., inbind9
, the error is caused by other files using C symbols that are not available in C++).This PR removes all flags that may cause this problem in all compilation commands.
When tested locally, it fixed the compilation error in
bind9
(with some manual fix on the fuzz target).Here is the command that contains
-Werror*
and causes this problem inbind9
: