- User Since
- Aug 19 2016, 10:21 AM (82 w, 1 d)
Fri, Mar 16
Super belated ping.
Thu, Mar 15
@rnk, does that make sense to you?
Yeah, you're right. Introducing a new linkage type for __attribute__((weak)) definitions is probably the way to go then?
Thu, Mar 1
"Hash the PDB"
"It is a usage error to feed a .dll file instead of a .dll to COFF linker"
Wed, Feb 28
Tue, Feb 27
Any updates here? We were hitting a similar error internally, which this patch fixed.
To clarify, this is only an issue when using type server PDBs, right? In other words, if we're purely cross-compiling using clang and lld-link, we wouldn't run into this, since clang-cl doesn't use type server PDBs (in other words, it does /Z7 instead of /Zi)?
Mon, Feb 26
The -g meaning -gcodeview for MSVC environments change should be a separate diff though, right?
Fri, Feb 23
We were affected by this too. Thanks for fixing!
Wed, Feb 21
Patch is missing context.
Feb 12 2018
FYI, binutils auto-import actually creates a fake IAT entry rather than using a dynamic initializer. I think it's actually a pretty cute trick. http://blog.omega-prime.co.uk/2011/07/04/everything-you-never-wanted-to-know-about-dlls/#how-auto-import-works has details. That only works when you're referencing an external imported symbol directly though, and breaks when you need an offset from the imported symbol.
Feb 8 2018
We've seen lots of spurious warnings from this. Thanks for the fix!
I had actually run into this issue, but in my use case the --export-dynamic-symbol should have arguably been a -u in the first place, so I just changed it there. I didn't realize the fix in LLD would be this straightforward though. LGTM.
Feb 7 2018
LGTM, but I'd like @majnemer to take a look, since I'm not very familiar with this code.
Feb 5 2018
This seems ... suboptimal. It breaks existing code in the sense that their code was already broken, and the compiler is just diagnosing it correctly now. Note that Apple themselves recommend casting and using a proper printf specifier: https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Strings/Articles/formatSpecifiers.html (under Platform Dependencies).
Feb 2 2018
LGTM with the inline comment.
Jan 31 2018
Some cases that drop the additional namespacing entirely, which is I think what @rjmccall was pointing out:
Jan 29 2018
Jan 27 2018
pin_ptr is technically not supposed to appear as a function parameter, and having it in that position can screw up demangling. Considering __strong is the default, it's probably not a good idea to map that one to pin_ptr; whichever qualifier is least likely to appear as a function parameter (though I don't know which one that is) should map to pin_ptr. The other lifetimes seem to demangle fine wherever you place them.
Jan 26 2018
I like this idea.
I'm putting this up for review mostly so that someone else can also confirm that link.exe always truncates section names.
(adding Rafael's current Phabricator account)
Jan 25 2018
Jan 24 2018
For future reference, you should upload diffs with more context (-U9999, or just use arcanist). This is trivial so it doesn't matter that much. It's also a good idea to add some reviewers explicitly, otherwise it's easy for things to slip through on the mailing list.
Jan 22 2018
LGTM, though I'll wait a few days before accepting in case Eric and Marshall have comments.
Jan 19 2018
Patch is missing context.
Jan 18 2018
With that said, I'd be fine with this patch as-is (once the critical section issue is fixed); we can always restore the inlining later whenever someone gets the time/if it does turn out to be an issue. libc++ on Windows will miss out on lots of inlining opportunities because of dllimport anyway (clang doesn't inline a dllimport function if it calls a non-dllimport function, which pessimizes lots of libc++ dllimport functions which call trivial non-dllimport helpers, e.g. basic_string::data calling _VSTD::__to_raw_pointer), so there are bigger problems already.
It doesn't seem like a big problem to me. Many of these APIs will result in system calls, and the cost of not inlining is probably dwarfed by the cost of the system call.
This prevents inlining, which might not be great. I'd be happier if we kept everything in the header but just forward declared whatever we needed from the Windows headers rather than actually including them (I've been meaning to do that for a while, just haven't gotten the chance to work on it yet).
Jan 17 2018
Seems like an oversight on their part, since these functions will be imported from VCRUNTIME*.dll for /MD. Oh well. LGTM.
Jan 16 2018
Jan 10 2018
Change comment to refer to existing one
Jan 9 2018
Should this be considered for picking to the 6.0 release branch?
Jan 8 2018
LGTM, given that it just seems like an oversight.
The warning was removed from -Wall.
Jan 7 2018
Not your fault. I think the right thing would happen if you added a Repository when uploading the change, and I'll update the instructions to reflect that. arcanist shouldn't be necessary. It's kinda unfortunate that Phabricator allows submitting a diff without having a repository ...
LGTM'd by Rafael on the mailing list: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180101/513595.html
Jan 6 2018
Yeah, that page mentions mailing lists to use for LLVM and clang, but doesn't mention other projects. The wording could be clearer there.
For future reference, the standard procedure is to add the relevant -commits mailing list (llvm-commits in this case) as a subscriber when uploading the patch, so that a mail gets sent out to the list. Over here the patch is trivial enough that I'm comfortable accepting regardless.
Jan 5 2018
Seems like a straightforward NFC refactoring. LGTM.
Can you add documentation for _LIBCPP_HAS_THREAD_API_WIN32 to docs/DesignDocs/ThreadingSupportAPI.rst? It should have been documented before, but this seems like a good opportunity to correct that.
I think LIBCXX_HAS_WIN32_THREAD_API would be more consistent with the existing configuration define names?
Jan 4 2018
Jan 2 2018
Looks like the blacklist files are installed using add_compiler_rt_resource_file, so at the very least, this would require updating getDefaultBlacklist in clang to look in the new location?