Thanks, that explanation looks fine. (And I agree that re-paragraphing it made more sense than my version)

Gentle ping: @rnk, are you satisfied with my changes and explanations from last week?

Oh yes, so there is. Moved the def.

Fixed a CI failure (unreachable in tokenizeWindowsCommandLineImpl was reached because I forgot to correctly handle an unquoted initial backslash in CommandName mode), and added an extra unit test to ensure the triggering case stays working.

Updated to fix the handling of a trailing unclosed quoted string, as suggested. It turns out (checking with that same 6-line test program) that even an empty unclosed quoted string still turns into an argv word, contrary to one of the existing unit tests.

In D122914#3423312, @rnk wrote:

We tried to write this code exactly following the rules the CRT uses to produce the argv[] array supplied to main. How does the CRT startup code handle that? Does it have special handling for the first token to match CreateProcess here?

Whereas I'm familiar with this code but not with the opaque-pointers effort, so I had to look up the other half of what was going on :-)

I like this version! This definitely says to me "nobody is going to just thoughtlessly append to an existing file".

While splitting up the test file is not ideal [...]

LGTM this time. Thanks very much for the rework!

I ran another test of this patch, by trying the cross product of the following cmake configuration choices (on a build targeting Windows, with LLVM_ENABLE_PROJECTS=clang):

I gave this patch a test against our downstream code, and found that it doesn't stop me needing the change I suggested in D119199: in clang/tools/driver/CMakeLists.txt, I had to change

if(CLANG_PLUGIN_SUPPORT)
  export_executable_symbols_for_plugins(clang)
endif()

so that the if statement reads

if(CLANG_PLUGIN_SUPPORT OR LLVM_EXPORT_SYMBOLS_FOR_PLUGINS)

Hi. This commit seems to have broken the following cmake command, on Windows (with HEAD pointing at 76cad51ba700233d6e3492eddcbb466b6adbc2eb):

cmake -DLLVM_ENABLE_PROJECTS=clang -DLLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON ..\llvm-project\llvm

The symptom is the same as @cristian.adam shows above:

-- SampleAnalyzerPlugin ignored -- Loadable modules not supported on this platform.
CMake Error at [...]/llvm-project/clang/cmake/modules/AddClang.cmake:185 (target_link_libraries):
  Utility target "SampleAnalyzerPlugin" must not be used as the target of a
  target_link_libraries call.
Call Stack (most recent call first):
  [...]/llvm-project/clang/lib/Analysis/plugins/SampleAnalyzer/CMakeLists.txt:8 (clang_target_link_libraries)

With HEAD pointing at the previous commit, clang/lib/Analysis/plugins/CMakeLists.txt does not add the SampleAnalyzer subdirectory at all, because LLVM_ENABLE_PLUGINS has the value OFF. But CLANG_PLUGIN_SUPPORT is ON, so now it does, and the above error happens.

In D116366#3215212, @aganea wrote:

Is that specific machine missing a header? Perhaps HAVE_SYS_MMAN_H isn't defined? @simon_tatham

Ah, now I see what you mean – I didn't look far enough!

I don't think so, I'm afraid. If you look at the definition of MacroInfo::tokens_begin() in clang/include/clang/Lex/MacroInfo.h, you see that it doesn't return a raw Token *: it returns a C++ iterator object.

Summarising what I said about argparse internally:

Improved the help text for the new variable.

I'm not sure it really makes sense to be able to override the output file location in the context of a build system – surely if you want to do that, you also have to tell cmake to change the OUTPUT parameter of the custom command, or the build products won't end up where the next build step expects them! Perhaps I should move this to after --mangling, though.

In fact, it turns out that libxml2 optionally uses zlib – it's a configure-time option. And I've been able to work around this issue another way now on the platform where I had the problem, by recompiling the libxml2 static library using configure --without-zlib, which doesn't seem to have lost any of the libxml2 functionality that llvm-mt needs.

Yes, that seems to have fixed it for me – thanks for the quick response!

In D105495#2882628, @tmatheson wrote:

I also think it's the caller's responsibility to make sure what they pass in is correctly signed.

(But I've pushed this patch anyway, because I'm reasonably confident of the answer; if it turns out that there's some case along those lines that I should have taken care of, I'll fix or revert.)

@lattner, thanks for the help. In this case, the real question is whether there's any use case for !srcloc that involves writing it out into a bitcode or IR file and then having a separate instance of clang load it back in again.

In D105491#2891860, @dexonsmith wrote:

although it'd be good to get someone more involved in lib/CodeGen to take a quick look / sign off (ideally someone that knows the use case for !srcloc...).

Addressed two nits.

... and removed an unused function from the previous version.

Split up the allocations as suggested.

Renamed types to [U]IntTy. This will affect some of the dependent patches too, but I agree that saving two characters is worthwhile – these names are long enough already :-)

Added an i64 !srcloc to the only existing test of them I could find.

I looked into this yesterday, and realised that I don't actually know what the use case is for emitting !srcloc metadata in an IR file.

This patch doesn't seem to have attracted much review attention in the last couple of weeks. On the theory that perhaps it's just too big and monolithic to review in one go, I've started to break it up into smaller pieces.

Good catch!

Hmmm. Two people have pointed out to me that my strategy of having a 32-bit SourceLocations::LowBits and an 0- or 32-bit SourceLocations::OptionalHighBits doesn't actually work, because an empty struct still takes at least 1 byte. So this version of the patch will still increase memory usage in the 32SL configuration, which is just what I was trying to avoid. Whoops.

Thanks to @miyuki for repeating the previous benchmark with this version of the patch (and on the same machine as before, which was better than I could have done).

@miyuki is working on other things at the moment, and I've picked this up.