Skip to content

Check *all* errors in LLVMRustArchiveIterator* API - #38676

Merged
bors merged 1 commit into
rust-lang:masterfrom
hanna-kruppe:llvm-check-success
Dec 30, 2016
Merged

Check *all* errors in LLVMRustArchiveIterator* API#38676
bors merged 1 commit into
rust-lang:masterfrom
hanna-kruppe:llvm-check-success

Conversation

@hanna-kruppe

@hanna-kruppehanna-kruppe commented Dec 29, 2016•
edited
Loading

Copy link
Copy Markdown
Contributor

Incrementing the Archive::child_iterator fetches and validates the next child.
This can trigger an error, which we previously checked on the next call to LLVMRustArchiveIteratorNext().
This means we ignore the last error if we stop iterating halfway through.
This is harmless (we don't access the child, after all) but LLVM 4.0 calls abort() if any error goes unchecked, even a success value.
This means that basically any rustc invocation that opens an archive and searches through it would die.

The solution implemented here is to change the order of operations, such that
advancing the iterator and fetching the newly-validated iterator happens in the same Next() call.
This keeps the error handling behavior as before but ensures all Errors get checked.

@rust-highfive

rust-highfive commented Dec 29, 2016

Copy link
Copy Markdown
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@hanna-kruppe

hanna-kruppe commented Dec 29, 2016

Copy link
Copy Markdown
ContributorAuthor

cc @dylanmckay@shepmaster
cc #37609

alexcrichton
alexcrichton reviewed Dec 29, 2016
View reviewed changes
Comment threadsrc/rustllvm/ArchiveWrapper.cpp Outdated

@alexcrichtonalexcrichtonDec 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should be #if, right?

Also, if this branch is taken, that should free ret, right?

@hanna-kruppehanna-kruppeDec 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops, yes.

@alexcrichton

alexcrichton commented Dec 29, 2016

Copy link
Copy Markdown
Member

Could we perhaps tweak this to check errors at the appropriate time instead? That is, only advance the cursor on calls to next?

@hanna-kruppe

hanna-kruppe commented Dec 29, 2016

Copy link
Copy Markdown
ContributorAuthor

We do only advance the cursor when calling next. The way the API works, next is effectively *it++, i.e., it fetches the current child and then advances the iterator. Simply flipping that (advance, then fetch) would skip the first child.

@alexcrichton

alexcrichton commented Dec 29, 2016

Copy link
Copy Markdown
Member

Oh right yeah what I mean is that we only advance the internal iterator when we need to actually fetch the result of the advance. I think we could store some boolean flags or something like that to avoid skipping the first child, right?

@hanna-kruppe

hanna-kruppe commented Dec 29, 2016

Copy link
Copy Markdown
ContributorAuthor

That would be pretty icky, but so is this hack. I'll give it a shot.

@shepmastershepmaster mentioned this pull request Dec 29, 2016
Closed
23 tasks
@hanna-kruppe
hanna-kruppeforce-pushed the llvm-check-success branch 3 times, most recently from 9883a01 to c99d716CompareDecember 29, 2016 19:36
@hanna-kruppe

hanna-kruppe commented Dec 29, 2016

Copy link
Copy Markdown
ContributorAuthor

@alexcrichton Updated.

alexcrichton
alexcrichton reviewed Dec 29, 2016
View reviewed changes
Comment threadsrc/rustllvm/ArchiveWrapper.cpp Outdated

@alexcrichtonalexcrichtonDec 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should happen first to prevent a segfault from calling next too much, right?

@hanna-kruppehanna-kruppeDec 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Hm yeah, it should be above as well, but it's needed here, too (I got segfaults without it).

alexcrichton
alexcrichton reviewed Dec 29, 2016
View reviewed changes
Comment threadsrc/rustllvm/ArchiveWrapper.cpp Outdated

@alexcrichtonalexcrichtonDec 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Perhaps this could go in an else above with the check to ensure it always happens?

@hanna-kruppehanna-kruppeDec 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good idea!

Robin Kruppe
Check *all* errors in LLVMRustArchiveIterator* API
8d50857
Incrementing the `Archive::child_iterator` fetches and validates the next child.
This can trigger an error, which we previously checked on the *next* call to `LLVMRustArchiveIteratorNext()`.
This means we ignore the last error if we stop iterating halfway through.
This is harmless (we don't access the child, after all) but LLVM 4.0 calls `abort()` if *any* error goes unchecked, even a success value.
This means that basically any rustc invocation that opens an archive and searches through it would die.
The solution implemented here is to change the order of operations, such that
advancing the iterator and fetching the newly-validated iterator happens in the same `Next()` call.
This keeps the error handling behavior as before but ensures all `Error`s get checked.
@hanna-kruppe

hanna-kruppe commented Dec 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Updated.

@alexcrichton

alexcrichton commented Dec 29, 2016

Copy link
Copy Markdown
Member

@bors: r+

Thanks!

@bors

bors commented Dec 29, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit 8d50857 has been approved by alexcrichton

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Dec 30, 2016
@alexcrichton
Rollup merge ofrust-lang#38676- rkruppe:llvm-check-success, r=alexc…
3e36dd8
…richton
Check *all* errors in LLVMRustArchiveIterator* API
Incrementing the `Archive::child_iterator` fetches and validates the next child.
This can trigger an error, which we previously checked on the *next* call to `LLVMRustArchiveIteratorNext()`.
This means we ignore the last error if we stop iterating halfway through.
This is harmless (we don't access the child, after all) but LLVM 4.0 calls `abort()` if *any* error goes unchecked, even a success value.
This means that basically any rustc invocation that opens an archive and searches through it would die.
The solution implemented here is to change the order of operations, such that
advancing the iterator and fetching the newly-validated iterator happens in the same `Next()` call.
This keeps the error handling behavior as before but ensures all `Error`s get checked.
@alexcrichtonalexcrichton mentioned this pull request Dec 30, 2016
Merged
bors added a commit that referenced this pull request Dec 30, 2016
@bors
Auto merge of#38697- alexcrichton:rollup, r=alexcrichton
7f2d2af
Rollup of 25 pull requests
- Successful merges: #37149, #38491, #38517, #38559, #38587, #38609, #38611, #38622, #38628, #38630, #38631, #38632, #38635, #38647, #38649, #38655, #38659, #38660, #38662, #38665, #38671, #38674, #38676, #38693, #38695
- Failed merges: #38657, #38680
@bors
bors merged commit 8d50857 into rust-lang:masterDec 30, 2016
@bors

bors commented Dec 30, 2016

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 8d50857 with merge 7f2d2af...

@hanna-kruppe
hanna-kruppe deleted the llvm-check-success branch February 2, 2026 22:14
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcrichtonalexcrichtonalexcrichton left review comments

Assignees

@alexcrichtonalexcrichton

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hanna-kruppe@rust-highfive@alexcrichton@bors