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.

Sign up for GitHub

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

Jump to bottom

Add c++ stdlib flag for clang++ only #11852

Merged
merged 2 commits into from
May 7, 2024
Merged

Add c++ stdlib flag for clang++ only #11852

merged 2 commits into from
May 7, 2024

Conversation

cjx10
Copy link
Contributor

@cjx10 cjx10 commented Apr 30, 2024 •
edited
Loading

Observing failure on building libtasn1 with jcc, suspect this flag used in clang caused the issue.

DonggeLiu
DonggeLiu reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@DonggeLiu DonggeLiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please also link the case you found that may caused this?
Thanks!

@cjx10
Copy link
Contributor Author

cjx10 commented Apr 30, 2024

Build log that indicates the failure: https://pantheon.corp.google.com/storage/browser/_details/oss-fuzz-gcb-experiment-run-logs/Result-reports/scheduled/2024-04-27-weekly-all/results/output-libtasn1-asn1_parser2array/logs/build/01.c-F1.log

@DonggeLiu
Copy link
Contributor

DonggeLiu commented Apr 30, 2024 •
edited
Loading

Build log that indicates the failure: https://pantheon.corp.google.com/storage/browser/_details/oss-fuzz-gcb-experiment-run-logs/Result-reports/scheduled/2024-04-27-weekly-all/results/output-libtasn1-asn1_parser2array/logs/build/01.c-F1.log

Thanks @cjx10, just a minor clarification for other reviewers:

We are not 100% sure this is caused by adding -stdlib=libc++ too early, but:

  1. The build can be built in OSS-Fuzz without JCC.
  2. As @mihaimaruseac pointed out in the comment, adding the flag too early is incorrect anyway.

We won't break anything fixing this, but it may help avoid some errors (possibly including this one).

@cjx10 cjx10 mentioned this pull request Apr 30, 2024
Closed
jonathanmetzman
jonathanmetzman approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@jonathanmetzman jonathanmetzman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

infra/base-images/base-builder/jcc/jcc.go

var bin string
if isCPP {
bin = "clang++"
// TODO: Should `-stdlib=libc++` be added only here?
newArgs = append(args, "-stdlib=libc++")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change makes sense to me.

@cjx10 cjx10 marked this pull request as ready for review May 1, 2024 23:45
@DonggeLiu
Copy link
Contributor

I don't expect this to break anything, please feel free to merge it once google/oss-fuzz-gen#235 confirms that.

It's OK if this does not fix libtasn1: They could be unrelated and we can leave that project for now, given it's likely a special case.

@DonggeLiu DonggeLiu merged commit 3524119 into google:master May 7, 2024
18 of 19 checks passed
@cjx10 cjx10 deleted the jcc_lib_flag branch May 8, 2024 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihaimaruseac mihaimaruseac mihaimaruseac approved these changes

@DonggeLiu DonggeLiu DonggeLiu approved these changes

@jonathanmetzman jonathanmetzman jonathanmetzman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cjx10 @DonggeLiu @mihaimaruseac @jonathanmetzman