Thanks for the review and merging this!

Thanks for the review and thanks to @hans for merging it!

@whitequark Suggested we add a API to let you create typed attributes, but was also happy with the change as is.

In D66144#1647091, @hans wrote:

I don't fully understand the severity here and what the status is. Is it needed for the 9 release?

+Tim who wrote r362128.

Ugh, I completely dropped the ball on this change.

It's in: SVN id 362128
https://github.com/llvm/llvm-project/commit/b7141207a483d39b99c2b4da4eb3bb591eca9e1a

Thanks for the review!

I can't comment to much on the cmake as my cmake-fu is weak, but from what I can tell it looks good.

Thank you for testing and your feedback!

Typo fixed, I only see one llvm-shlib/CMakeLists.txt file.

Ugh, I see some types, let me respin.

This should fix it, doing a build now.

Refactor loop

Okay, I have updated the patch now with the suggestions from @stella.stamenova, thanks! Please try it out, I'm currently running a build myself using ninja to make sure that it still works.

Thanks! As I don't have commit access I will need somebody to land this, thanks!

@hans @serge-sans-paille I'm re-opening this again as the fixes have landed. Build tested the latest master with the change on.

@serge-sans-paille I do not, so that would be greatly appreciated.

Thanks, this should fix all concerns raised, I tested this as well and seems to work.

@hans @serge-sans-paille So I built the package and has verified that the LLVM-C.dll is included in the package but I could not find the LLVM-C.lib file tho. After some hours of digging through the multitude of *.cmake files I have to admit defeat on how to get that in there, any pointers would be very much welcome.

Have now tested this patch and confirmed that it is working for me at least. Needed to make the libs argument optional or the python script would error out. I did not see any sign of $(Configuration) in my scripts.

Should address all of the comments, haven't done anything the $(Configuration) @hans saw in the error message. Well setup something to test this.

And remove the parentheses.

Not tested, will address last comment and then setup something I can test this on.

In D56781#1359729, @hans wrote:

I actually tried the patch and got

Updated changes with comments from @serge-sans-paille, thanks!

No I don't have commit access, so please feel free to commit. Will this land before the branch point and also make it out in that release?

The C-API changes looks good to me and the logic makes sense. Just one nit.

This can be closed, as changes has landed in a different review. See D45100.

In D35077#1191123, @hans wrote:

In D35077#1190945, @xbolva00 wrote:

In D35077#1190698, @Wallbraker wrote:

Can this please please please be pushed ASAP so it can go in to the 7.0 release?

ask @hans if he can merge it to 7.0 branch :)

I'd prefer not to merge it. Since this is a new feature that's been in the pipe for over a year, I don't think we should rush it in now. It would be better to let it land safely on trunk, get any kinks worked out and then have it be part of the next release.

We should also check whether this will become part of the installer automatically, you can see how its built in utils/release/build_llvm_package.bat, or if that needs some update.

Can this please please please be pushed ASAP so it can go in to the 7.0 release?

See D45100.

This has is already in the C API now, so closing.

That should be all of the comments done.

Updated with @bhelyer changes and updated to latest master.

Another update of the commit for llvm master.

Added people who seems to be doing stuff with cmake on windows.

Updated the diff to master, haven't build tested yet.

Thanks for testing again @Memnarch, I'm going to update this changeset soon and then try to get it included in LLVM Weekly.

This is a NFC update to the patch, bringing it up to master. As I wrote above I can't test it, but should work just fine.

No sorry, I moved and started a new job. And don't have access to the windows machine for a while.

Ping?

I don't have commit access so I need somebody to commit this for me, it has been like a month since I got the first LGTM on this review.

In D35077#863504, @Memnarch wrote:

That looks much better. When in doubt, you can always download the revisions from the most recent(or any other diff) to make those changes based on those.

This should not have any other changes, I think. Problem is I do not know which revision the old one was based on.

In D35077#863378, @Memnarch wrote:

In D35077#863339, @Wallbraker wrote:

I have updated the commit to use for locating the libs CMAKE_CFG_INTDIR and should fix the issue @Memnarch saw. I only have build tools installed so I don't know if I can test it under a Visual Studio like environment.

Cheers, Jakob.

Can you check your diff? Expected changes in "tools/llvm-shlib/CMakeLists.txt" only but all files are modified with other changes(compare the new diff with the previous one)

I have updated the commit to use for locating the libs CMAKE_CFG_INTDIR and should fix the issue @Memnarch saw. I only have build tools installed so I don't know if I can test it under a Visual Studio like environment.

Thanks for the review, I do not have commit access so somebody needs to commit this for me.

In D35077#813499, @beanz wrote:

I don't think this should be added as a new tool. We support generating LLVM-C.dylib on Darwin via the tools/llvm-shlib tool. It would be better if this were added there instead of completely separate.

Ping?

Hey cool that you are doing this work, finally!

Only spotted one case of a Value not being cloned. Others use DeclareBB.

I think that's the last of the changes needed for the echo.cpp files, can we agree on something for the LLVMBuildCallInPad function and have somebody that knows more of what the IR should look like comment on the IR file.

Last update from Bernard.

Fixes for Core.[cpp|h] files.

Added the CloneValue call and it now passes the test again. I had to add
ConstantTokenNone and ConstantNullPointer to the clone_constant_imple
function.

Adding newline to end of file.

Updated diff from Bernard.

This works for my test case as well, nice work! :D

This commit compiles, fixes my test case and my full code now links correctly! Awesome thanks.

This change gives me linking errors when I try to compile it on git head.

I would like to see something that adds the integer and string attributes that @jyknight brought up, so we know they work before we commit to this API.

Considering how long the other Attribute review is taking just remove the function all together, and put it in once we actually have the finished interface down.

I kind of prefer the AttributeSet approach, but don't feel to strongly about it.

I strongly feel that we should just add new function and then remove the old ones the release after.

Looks good to me!

So they are completely redoing[1] the debug info interface which this is a precursor for, so I dunno how much benefit this will give us right now.

I don't know that much about llvm internals, but this seems to make sense.

As I was reading this patch and comments I had some concerns about the different behavior between bfd and gold and that error messages would not provide enough information. The patch seem to address these concern so I'm happy with this patch.

I thought we came to the conclusion that we should add new functions and not change ABI on the old ones. Yes this will mean copies of the functions but its only for a release.

Looks good, thanks!

I'm not sure that this is worth the effort and code churn.

Maybe not the links but put in the comment that its function/arg/return attributes that this functions returns, and mention that they are listed in the language reference.

About the code:
The function requires documentation. Is there a list of attributes somewhere? What does it return on failure? How long are the values returned valid? Per context, per thread, per process, per release?

Whelp that was a bit fast.

In D18727#391285, @rnk wrote:

First, you're breaking compatibility here with old programs using LLVMAttribute. We should ask Jeurgen about that. If we decide that LLVM's underlying attribute handling has changed so drastically that we're OK with a break, then we should delete the LLVMAttribute enum.