C API changes LGTM.

I think deleting functions with proper alternatives is fine, probably one at a time though so people can work through updating calls to those functions.

LGTM

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.

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

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.

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

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.

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?

In D85981#2218583, @vsapsai wrote:

C API changes LGTM. My one request is to update the echo llvm-c-test to use the new API.

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.

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.

C binding changes LGTM. Don’t worry about changing them, they are marked as experimental and subject to change at any time.

@MaskRay I have a refactoring in https://reviews.llvm.org/D59425 that covers that.

Y’all can revert it. I’ll resubmit the patch with updated tests tomorrow.

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.

Document the lifetime of the context parameter

[NFC] Rebase

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

LGTM

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

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