In D92160#3485400, @Kale wrote:

In D92160#2449507, @dexonsmith wrote:

But it's possible we don't need this. If it's safe for us to update the tests and make FileManager::getFileRef always canonicalize to an absolute path, that would definitely be cleaner. FileManager::makeAbsolute can use whatever the FS's CWD is at the time of query... nice and clean.

I've tried to fix this bug through this way but found that "making FileManager::getFileRef always canonicalize to an absolute path" was pretty hard, because there are many test cases using states stored in FileManager and assuming that they are all in relative form. Besides, the assumption that "the CWD won't change" indeed is correct by design and works fine for single compiler execution. We might not change that unless for a strong necessity.

So I personally believed that this bug is caused by libtooling's incorrect use of FileManger. I plan to fix it by implementing a class like LibtoolingFileManager, or AbsoluteFileManager, which extends the original FileManager and using abs path as key to store the status of seen files, but only used for libtooling.

I will try to upload a patch later to verify the possibility.

• Add FIXME in test case.
• Add discourse topic link in summary.

I agree with @keith to commit this patch without using on-demand-parsing through cc1.
As this patch has nothing to do with the target triple issue we found.

ping

ping.

ping

I think I have found out the reason for the problem, and it proved my guesses.

It seems that it is not committed on my behalf. Maybe you have forgotten to add me as the author?
Sad : (

Could you please commit it on my behalf (Ella Ma <alansnape3058@gmail.com>)?
Thanks a lot.

Updated as suggested.

To make it work on Windows, Linux, and Mac OS, using echo to create the external function map, and using AST file for CTU analysis.
Tested on Windows, Linux, and Mac OS under x64.

Replace on-demand-parsing with loading AST file for the new test case.
Tested on Linux and MacOS(x86).
If it can also pass the CI test on Windows, I think we can have another try on landing this patch.

In D115716#3217786, @Szelethus wrote:

First off, your patch is great, and I'm pretty sure we want it!

I remember working around here, and I either have never quite understood what makes exampleReport an example, or have long forgotten. Can we not just rename it to chosenReport, or simply report?

I have confirmed that this problem is not due to this patch.
Besides, on Mac, both m1 and intel, the on-demand-parsing as well as loading an AST file generated via driver argument -emit-ast will also trigger this problem.
However, when loading an AST file generated via cc1 argument -emit-pch, the problem is not triggered.

In D102669#3206089, @steakhal wrote:

Prior to this patch, it worked on M1; after landing it broke something, so we clearly shouldn't land this.

In D102669#3199270, @steakhal wrote:

We shouldn't skip mac targets. I CC ASTImporter folks, they probably have an M1.

It seems that it is not this patch that triggers the problem, which is similar to D75665.
IMO it is the problem of on-demand-parsing, but I do not have a Mac M1 device to reproduce this bug.
Maybe we can just land this patch by restricting the test case to be executed only on Linux, just as what D75665 does (rG5cc18516c483 vs rG97e07d0c352c), and leave the problem for future fixes.

Could you please revert this on my behalf? I currently have no idea to fix this problem.

I do not know how this error happens. Maybe we can currently revert this patch an have another try in the future.

It seems this patch has nothing to do with the failure in the Linux build. I think it is now ready to land.
Thanks a lot for your suggestions during the revision.

Fix YAML:1:293: error: Unrecognized escape code error by replacing lit substitution pattern %S to %/S.
Fix cp problems by removing the file copy operations.

When running my test case ctu-lookup-name-with-space.cpp on Windows, llvm-lit reports 'cp': command not found. And this is the reason why it fails on Windows.
And when I remove the cps and replace them with original file names, clang reports YAML:1:293: error: Unrecognized escape code, it seems that the static analyzer only reads compilation database in YAML format on Windows.
Should I disable this test case on Windows? Or is there any other approaches to make it work on Windows?

1. Revert function CrossTranslationUnitContext::getLookupName
2. Add length when dumping the CTU index mapping via function createCrossTUIndexString
3. Remove the assertions during CTU map query process
4. Make function parseCrossTUIndexItem more readable

What do you think about this patch? Can it be landed now? Or I should debug the crash in the Windows version detected with the previous version of my patch.

1. Fix formatting bugs
2. Update lookup name format as <USR-Length>:<USR> <File-Path> in all comments and documents
3. Add the new test case as a part of clang/test/Analysis/func-mapping-test.cpp to verify the lookup name
4. Change the macros for asserting the lookup name format to an NDEBUG wrapped function

It seems that this issue has better solutions outside the CTU module, I will drop this revision as mentioned above.
Sorry for the delay.

It has been a long period since the last discussion, I hope you can still remember this bug. And apologize for the delay.

Update the test case to avoid a crash in the Windows version of the c-index-test tool.

In D101763#2777821, @steakhal wrote:

It seems like everything passes. Yeey, good job!
Shall I commit this tomorrow on your behalf?

Re-submit the patch to re-build the code.

Re-submit the patch to re-build the code.

In D101763#2768549, @martong wrote:

First of all, thank you for the patch!
We had a meeting with my colleges (@steakhal, @gamesh411) and we agreed in the following. This is indeed an issue and the fix is okay. About the test, we'd like to ask if you could create a small test lib with its own 'open' function that would simply call the system's open and would print 'open'. And then with LD_PREOLOAD you could use that lib in the lit test. (If that approach is not working for some reason then we may just mark the test with XFAIL.)

In D102149#2768541, @martong wrote:

Thank you for the patch!

Though, the idea is nice, there is a serious technical obstacle here: we cannot use the clangTooling lib as a dependency of the CTU lib because that would introduce a circular dependency. Actually, this was the reason for introducing the invocation list yaml file; we could not use the compilation database implementation from tooling (https://reviews.llvm.org/D75665?id=260238#inline-722328).

In D102062#2768569, @martong wrote:

First of all, thank you for the patch!
We had a meeting with my colleges (@steakhal, @gamesh411) and we agreed in the following.

We'd like to keep the current behavior because this way the behavior is similar that we got used to with any other command line tools.

In D102149#2765402, @martong wrote:

Thank you @OikawaKirie for working on this many CTU related patches! I am going to find time for a thorough review and going to pursue @gamesh411 as well to do the same! On the other hand, it would be really useful if you could build a "Stack" from these patches, I mean could you please set which patch depends from which other (see "Edit related revisions")?

In D102669#2765233, @steakhal wrote:

I'm not really familiar with the extdefmap part, but I'm surprised that we are using spaces as separators.
Shouldn't we consider using a different character?

I prefer the idea of changing the delimiter character, but it may lead to modifying a lot of test cases.
I think we'd better make this change in another revision in the future if we do want to change it.

It seems impossible and not so reasonable to eliminate all white space characters in the USR as mentioned in the test case of revision D102669.
This patch is split to revision D102669 to fix the wrongly parsed CTU index file and revision D102614 to handle the member pointer type mentioned here.
Please continue with these two following revisions and I will close this one.

In D102159#2749033, @akyrtzi wrote:

Maybe we could also handle this kind of type instead of leaving it 'unhandled'? What Type is it?

FileCheck error: '<stdin>' is empty.

Monitor function open together with openat to fix the failure in the test case.
As some distros actually call function open rather than forward to function openat, both functions should be monitored by strace.

In D101763#2741629, @steakhal wrote:

Okay, so you 'just' want an indication for the given open call. What about using strace?
strace -e trace=openat %clang_cc1 ... 2>&1 | grep '"invocations.yaml"' | FileCheck %s

In D102062#2744267, @steakhal wrote:

On second thought the current behavior is reasonable.

In D101763#2743984, @steakhal wrote:

By inspecting the output that is piped to the count, I have a suspicion:

openat(AT_FDCWD, "invocations.yaml", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)

Sorry for the spam.

• Update the test case as suggested.
• Add a case branch for the index_error_code::success error code in function IndexErrorCategory::message to silent the compiler warnings.

In D101763#2741536, @steakhal wrote:

Overall, it looks promising. But I don't quite get this test.
There is no invocation yaml in the temp directory. So, you are probably not testing the right thing.
You wanted to test if the invocation yaml exists, and could be opened but the parsing fails.
You should demonstrate that when a parsing error happens, the error code has recoded and it won't try to reparse the invocation yaml again and again.

Add a regression test case by mocking the open function. When this function is called with the file name of the invocation list, the mocked open function will reject the open operation and dump a log. And we will then check how many times are the invocation list opened. (Thanks to the ideas by steakhal in another patch of mocking a library function :-).)

Ping @sammccall again for landing this patch on my behalf.
Thanks.

If this patch is OK, could you please commit it on my behalf (Ella Ma <alansnape3058@gmail.com>)?
Thx.
And I will continue working on the -Xanalyzer option for clang-check, as it is too complex when adding analyzer options via the -extra-arg option.

In D83660#2675064, @mikhail.ramalho wrote:

Indeed it looks like a copy & paste error, I'm surprised no one found it earlier.

Regarding the tests, we used to have make check-clang-analysis-z3 (or something similar) that would run only the analyzer's tests, but using Z3 as the constraint solver. It looks like this change broke it: https://reviews.llvm.org/D62445

ping @sammccall again

In D83660#2661369, @martong wrote:

BTW, I was obstructed by the z3 requirement in the regression test case when I tried to understand your test case. Even though I set the variables LLVM_Z3_INSTALL_DIR and LLVM_ENABLE_Z3_SOLVER during CMake, and the CSA can be correctly compiled with Z3, I still cannot make the test case run during make check-all. Therefore, this case was manually executed with llvm-lit -DUSE_Z3_SOLVER=1. Could you please tell me how to enable Z3 during llvm-lit.

I am a bit unhappy that we cannot run the test automatically, but maybe in the future.
@steakhal, https://reviews.llvm.org/D83677 seems to be related, should we push that?

In D83660#2597025, @steakhal wrote:

I'm somewhat bothered that this patch is not landed yet. :|

I made a crude mock to trigger the bug using LD_PRELOAD.

LD_PRELOAD-workaround.patch2 KBDownload

The test reproduces the issue and passes if you apply the fix from this patch.

However, it requires compiling a shared object with the same compiler you used for building clang. I'm using the c++ compiler, for now.
I think it's a good starting point.

In D98918#2636735, @steakhal wrote:

@sammccall ping.

In D98407#2619177, @njames93 wrote:

Matchers change over time, so it may be wise to create a script to parse clang/lib/ASTMatchers/Dynamic/Registry.cpp and extract the matcher definitions from there. Save manually updating of this.

In D98407#2619177, @njames93 wrote:

Matchers change over time, so it may be wise to create a script to parse clang/lib/ASTMatchers/Dynamic/Registry.cpp and extract the matcher definitions from there. Save manually updating of this.

Replace concatenating the output from clang-check and plist with separated checks to workaround the test failure on Windows.

1. Update option name from -output to -analyzer-output-path. As it can either be a file name or a directory name, the word path may not be accurate. Please give me your suggestions on this option name.
2. Add more verifications on the output files, rather than verifying the value of the -o option. For simplicity, the regression test case only checks the plist format.

In D97265#2581653, @sammccall wrote:

My only concern is the recursive call to this function on line 1359. If you think it is also OK for the recursive call on line 1359, I will update the patch as you mentioned above.

If the revision is done, please commit it on my behalf. Thank you.

If the revision is done, please commit it on my behalf. Thank you.

Split to

• D97251 (clang/utils/TableGen/ClangAttrEmitter.cpp)
• D97185 (llvm/lib/DWARFLinker/DWARFLinker.cpp)
• D97254 (llvm/lib/LTO/ThinLTOCodeGenerator.cpp)
• D97255 (llvm/lib/Support/VirtualFileSystem.cpp)
• D97258 (llvm/tools/llvm-link/llvm-link.cpp)

As a recursive function it is, the original patch in D91844 was adding assertions where the function is called other places in the file.

In D92634#2477342, @danielmarjamaki wrote:

However, the mainstream compilers like GCC and Clang implement this as the overflowed value, and some programmers also use this feature to do some tricky things.

hmm.. you mean if some -fwrapv flag is used right. yes I should disable this checking then.

For information, I am not very worried about signed integer overflow. I am mostly worried about the compiler optimisations. If the compiler determines that there is signed integer overflow in a execution path and removes all code in that path.

I was inspired by this blog post: https://www.airs.com/blog/archives/120

This function:

int f(int x) { return 0x7ffffff0 < x && x + 32 < 0x7fffffff; }

might be optimized to:

int f(int x) { return 0; }

In D92634#2476161, @danielmarjamaki wrote:

Besides, the return value should be the exact value computed from the two integers, even unknown, rather than undefined. As the developers may overflow an integer on purpose.

I am not sure what you mean. If there is undefined behavior then the value should be undefined and nothing else.. right?

I think it could be better to implement this check with a checker on PreStmt<BinaryOperator> and so on. And IMO, checkers have enough functionalities to report these problems.

Replies from the original author Hao Zhang <zhanghao19@ios.ac.cn>

Replies from the original author Hao Zhang <zhanghao19@ios.ac.cn>

Replies from the original author Hao Zhang <zhanghao19@ios.ac.cn>
(Sorry for the wrong email address in the previous reply.)