- User Since
- Apr 17 2017, 7:43 PM (209 w, 1 d)
Mar 9 2021
I believe removing inode numbers is the correct fix, yes. The workaround I applied, and the one here, are both insufficient in the general case.
Sep 26 2020
@cdisselkoen I believe we were talking past each other in the other revision - sorry about the confusion I may have caused. Hopefully this is clearer.
This has already been committed, it should remain closed.
Sep 25 2020
Aug 28 2020
We have tested this proposed change out on our CI systems and have seen no relief from the symptoms of inode reuse with this approach. Abandoning this revision in favor of a more narrow fix.
Aug 19 2020
Aug 18 2020
@aprantl Good idea. Updated.
Switched tactics here. Rather than just change the source of the entropy, let's increase it from just inodes to (64-bits of inode) plus (file size) plus (mod time). It is still possible to defeat this scheme, but it means an attacker would have to replace the PCM with one that has been padded out to the same size then backdate its modtime to match the one in the cache - or some cascading failure of the syscalls providing these data conspires to make this happen.
Aug 17 2020
Figured it out for myself. The test is forming paths that are using non-canonical path separators. Naively using those as keys means that the subsequent canonicalization done by the ASTWriter renders the keys useless for lookups into these structures. I'm going to switch to FileEntry::tryGetRealPathName since it's quite literally what ASTWriter is doing as part of its canonicalization phase. I worry about that as a solution in general though. In the future, it would be great to expose the canonicalization utilities in the ASTWriter as a more general kind of facility that could be shared between the implementations so we don't desync things again.
Aug 15 2020
Okay, I'm done throwing revisions at the bots. This windows-only failure is bizarre. @rsmith Do you have any insight into what's going wrong here?
Aug 14 2020
Jun 9 2020
C API changes LGTM. My one request is to update the echo llvm-c-test to use the new API.
May 31 2020
I see you’ve already abandoned the revision, but I want to say that if you wish to do this, please make a separate entry point in the C API for creating scalable vectors. We are still committed to ABI stability in the core bindings.
May 1 2020
We are committed to ABI stability in the core bindings. If you wish to pursue this change, you will need to seek consensus on the mailing list. I'm personally not opposed to an ABI break, but it *must* be done in the open.
Why not change the behavior of LLVMGetMetadata instead? It's an ABI-compatible change, and expanding the API to cover more cases instead of crashing is API-compatible.
Mar 11 2020
Dec 27 2019
Nov 21 2019
C binding changes LGTM. Don’t worry about changing them, they are marked as experimental and subject to change at any time.
Oct 23 2019
Oct 22 2019
Aug 14 2019
Y’all can revert it. I’ll resubmit the patch with updated tests tomorrow.
Aug 10 2019
Jul 26 2019
Jul 24 2019
Jul 23 2019
Jul 22 2019
Jun 14 2019
We spoke out of band about the troubles I’m having verifying this locally, but the patch looks good and my limitations shouldn’t hold it back any longer.
May 25 2019
Document the lifetime of the context parameter
Apr 24 2019
Apr 22 2019
Apr 17 2019
Apr 16 2019
@jberdine Any further comments?
@whitequark Could I get your stamp of approval as well?
This patch doesn't apply cleanly to my checkout, so I'm going to fold it into D60725.
Apr 15 2019
@jberdine This patch handles the global variable side of things. You should be able to replace those accessors in Core with a call to LLVMGlobalCopyAllMetadata. You can filter for !dbg and dig out the global variable expression. From there, you locate from global variable expression to global variable with LLVMDIGlobalVariableExpressionGetVariable and from global variable to line with LLVMDIVariableGetLine.
A DISubprogram is a kind of DIScope so you can use LLVMDIScopeGetFile and the rest of the accessors to poke at their metadata. For Global Variables, the idea should be to add corresponding accessors for the scope, file, name, source, and directory since these nodes do not store DILocations. It seems like the latter is pressing, so I'll make a patch for it in a bit.
Apr 10 2019
Add an accessor for the "inlined at" location
All this is reminding me I need to go back through and remove this header. The Go bindings do not need special treatment, and any new bindings they have need to be merged into LLVM-C
*sigh*, I'll resubmit this and nuke the ones in the Go bindings then.