User Details
- User Since
- Jan 2 2013, 4:34 PM (559 w, 4 d)
Thu, Sep 21
Let's close it.
Closing, we left the test alone, it still uses -fuse-ld=lld-link2. Perhaps in the future we should reconsider this, but that's how things stand now, and we aren't going to land this patch as is. + @MaskRay , who has thought about -fuse-ld= semantics.
Mon, Sep 18
+@cjdb , do you have time to review this?
Fri, Sep 15
lgtm
Thu, Sep 14
lgtm
Wed, Sep 13
One of our use cases for having a "large" attribute for global data is to have a way to mark instrumentation data as large. We would use this for ASan global metadata and PGO name data, which are not accessed frequently during program execution, but contribute significantly to binary data size.
FWIW after speaking with @thakis offline, I do think we did "everything right", in the following sense. We added the deprecated attribute to the class and left it there across one full release (roughly 1 year). This should leave enough time for people to notice the warning when updating libc++, turn it off temporarily with -Wno-deprecated as they fix their code, and then re-enable the warning. I think this is the right model we want to give folks.
Thu, Sep 7
Following up, the vararg fix is here: https://github.com/llvm/llvm-project/pull/65692
Wed, Sep 6
The reproducer in the commit message no longer asserts for me, maybe we can close this.
Fri, Sep 1
lgtm, thanks!
Thanks for the patch, this is gnarly code. At the time I was probably thinking that flat vectors are good for performance, and that we'll have tons of line table entries, but as the inline location handling evolved, it's gotten quite complicated.
Thu, Aug 31
lgtm
I think that's reasonable. libgcc probably retains them out of some desire for longer term backward compatibility, and I think we're a little less constrained there.
+1, this use case has long been addressed by the assertion mode, and then the new safe and hardened modes.
Wed, Aug 30
I haven't heard any serious concerns, and I hope I addressed your questions. I plan to go ahead and land this, but feel free to modify or revert if you have concerns.
I wanted to follow up on this, it was hiding at the bottom of my inbox.
At various points I have tried to untangle the meaning, purpose, and function of these various stack adjustment routines, and I never felt like I had a satisfactory understanding. I trust that you got to the bottom of this.
Tue, Aug 29
This seems like it will generate warning cleanup work for vendors, so I'll mention #clang-vendors , but I think the code looks good. I don't think it makes sense to add a fine-grained diagnostic category to allow people to clean up incrementally in this case.
I think this kind of error ("relocation against symbol in discarded section") is really hard to debug, and probably deserves a whole page of documentation. I think the best option would be to emit a short link to online documentation describing the issue category in detail, something like https://lld.llvm.org/missingkeyfunction.html .
I put together a fix here: https://github.com/llvm/llvm-project/compare/main...rnk:llvm-project:fix-vararg-align
+@rupprecht @aeubanks, can you review this Bazel change?
I need to get to it, my recollection is that @mstorsjo ran into the same issue here for mingw and made some changes, I wanted to go dig those up as a starting point. I may have completely forgotten things though.
lgtm
Mon, Aug 28
lgtm
+@efriedma, since I think we had that ongoing thread about what to do with i128 alignment in data layout. Is this a reasonable way to handle the datalayout migration?
Regarding large code model jump tables, I think I filed an issue for alternative, relocation-less jump table encodings: https://github.com/llvm/llvm-project/issues/62894
lgtm with some tweaks to the test.
It took me a while to find this code to understand the issue. The problem is that there is an inconsistency between the GVA linkage and the LLVM IR linkage for these constructors. In this godbolt example, ??0?$TInputDataVertexModel.. is marked internal and marked comdat, and that is uncommon. PGO instrumentation appears to do the wrong thing in that case. Is that more or less what's happening?
Aug 23 2023
Thanks, I think this is a clang bug. As I understand MSVC's behavior, we should not pass highly aligned variadic arguments indirectly: https://gcc.godbolt.org/z/Kr67xWTeE
Aug 22 2023
I went ahead and pushed a revert of this, since this change was pretty disruptive, at least in our experience. I could be convinced that it's worth sunsetting this extension for accepting C-style format string macro code, but that doesn't seem like the original intent of this change. Maybe that's the right thing to do, but it should be an intentionally considered change.
lgtm, if @asl is offline, and it's useful
I'm not that familiar with the typical compatibility requirements for DLLs in the Windows system folder, but it seems pretty disruptive to ship a DLL in the system folder, and then ship ABI incompatible changes without increasing the version number. Wouldn't this all work if the STL increased the version number of msvcp140.dll instead?
Aug 21 2023
lgtm, thanks!
Aug 18 2023
lgtm
lgtm
- rebase
Aug 17 2023
Ping, any outstanding concerns?
Thanks for the review, hopefully all comments are addressed.
- apply fixes
Also assuming that /Ze is similar to our -fms-extensions which I believe is the case, but not completely sure
So, I'm strongly in favor of the feature, but I want to get feedback on the name.
Seeking a code reviewer, +@aeubanks, @vitalybuka
I don't think this is the right thing to do, I think I recall saying as much on some other review. The MSVC docs say that /Za//Ze control _MSC_EXTENSION, and as I understand it, /Za is more like our -fms-compatibility flag, so it makes sense to control this macro with -fms-compatibility.
Aug 16 2023
lgtm, thanks!
That doesn't handle the second of your test cases though, where dllimport is put on the member directly:
...
It's not clear to me how we'd want to handle that. I don't think it comes up in libc++, and I can't think of any code that would want to do that either, really.
Aug 14 2023
So, as I understand the discussion, after this patch, it will still be possible to get dwarf and codeview in the same compile, but users will have to resort to cc1 flags.
Aug 12 2023
Aug 11 2023
I think MSVC defines _MSC_EXTENSIONS under one of its compatibility modes as well, so it's not a good indicator of "is this compiler MSVC-like". I think we should be exposing the __cpudex builtin any time we are being MSVC-like, not under fms-compatibility. Would that make things sensible again?
What are the next steps here? Have concerns been addressed? The CI failures seem unrelated (libcxx tests is an AIX bot out of disk, clang CI is an interpreter test that seems unrelated, but please confirm).
Thanks! My concerns are addressed, but please confirm that Eli's are too.
This looks correct to me.
Aug 10 2023
You can see where the eh cont table was added here: https://reviews.llvm.org/D99078
Here are some comments, I have to run, so it's not complete, but hopefully still useful.
lgtm
lgtm
Thanks!
Aug 9 2023
I agree that we can't emit all labels with the .L prefix into the object file symbol table. That would be prohibitively expensive.
This approach makes sense to me.
Aug 2 2023
Jul 31 2023
Jul 28 2023
Thanks!
Jul 27 2023
- rebase
- rename call lowering file