Pavel's test case passed on my local machine (x86_64 linux workstation).

Hi Pavel,

I had to revert this change because the test case broke the windows builder. What's the right way to update/mark the test case so that it is only run on appropriate architectures & operating systems?

I did leave 'main' in the .s file, but it's not very big. Is this ok now?

Update test case to use lldb on .o file and not run the inferior.

Pavel, your last comment lost me completely. How can I test the code I added to lldb if I don't run lldb? I am a complete newbie at writing test cases so there's probably something basic I'm missing? How would I observe the value of the variable without running lldb? Also, if the program doesn't contain 'main', won't the compiler complain? Is there an existing test that does what you're suggesting that I could look at?

Add a test case.

It has taken a bit of time to get through the GDB reviews, but the change to GDB was accepted and committed: https://sourceware.org/pipermail/gdb-cvs/2021-April/050267.html

In D97786#2607275, @labath wrote:

In D97786#2603656, @cmtice wrote:

I'm not sure about using target.debug-file-search-paths, and what the changes Pavel is suggesting would entail.

I imagine it would involve calling Symbols::LocateExecutableSymbolFile to locate the dwo file, similar to how we do that for dwp files (see SymbolFileDWARF::GetDwpSymbolFile). (Disclaimer: I have not tried doing this, so I don't know if that function would work out-of-the-box.)
Note that I myself am not convinced that this is the right solution (that function is rather heavy), but it does solve the problem of "how do we let the user specify/choose the location of the dwo files" (answer: put the path in the target.debug-file-search-paths setting), and it would search the cwd and the module directory automatically. And the "heavy-ness" of the function is moderated by the fact that it is a no-op for absolute paths to existing files.

But the real question I am trying to resolve here is not "what are all the places we should be searching for the .dwo files?" but "When we're given a *relative path* in the DWARF for finding the files, what should it be relative TO?".

I'm sorry, but what's the difference between those two questions? Is it about the fact that the second question sort of implies that there should only be one correct location where we should be searching?

Update to incorporate Pavel's suggested simplification.

I'm not sure about using target.debug-file-search-paths, and what the changes Pavel is suggesting would entail. But the real question I am trying to resolve here is not "what are all the places we should be searching for the .dwo files?" but "When we're given a *relative path* in the DWARF for finding the files, what should it be relative TO?".

It is true that the compiler will not know where the final executable will be placed, and if the executable gets moved then debugging with .dwo files will probably not work anyway, However as it is currently written, LLDB will fail to find the .dwo files, *even when the binary has not been moved*, if the debugger is launched from any directory other than the one containing the binary.

Upload the correct patch this time. (sorry!)

Something went very wrong on the patch upload. This does NOT look like the
patch I intended!

Fix small typo in comments.

Remove quotes from -NOT tests.

Fix NODEBUGINFO-NOT test.

Remove period from help text. Fix tests.

Rename flag to -fdebug-default-version; add help text, mentioning DWARF. Update tests to use -### and add assembler tests.

Yes, I will change the flag name back to debug-default-version, and add help text mentioning dwarf. I'm in the process of trying to re-do the test cases as required (I'm a bit new to this so it's taking me a bit to figure this out).

Make second call to ParseDwarfDefaultVersion unconditional (to match the first and avoid errors with -Werror).

For the record, I double-checked and if we use the flag and don't check for it (i.e. if we move the parsing inside the EmitDwarf is True block) then -Werror does indeed complain about an unused command line argument.

dblaikie: The code, as written, parses the dwarf default version flag, whether or not DWARFVersion is 0, i.e. whether or not a -gdwarf-N flag was passed. It only USES the value if there's no overriding value.

re-try upload again.

Ok, I think the upload was correct this time.

Hmmm...I'm having upload issues.

Try uploading correct diff file?

Made requested changes:

• renamed option to be dwarf-specific
• fixed spelling & blank line issues
• only set version if emit-dwarf is true
• move test to Driver directory

Made requested formatting changes.

In D54348#1531751, @bcain wrote:

In D54348#1531741, @cmtice wrote:

Ping? Is anybody looking at or working on this? The bug this fixes is one of the issues currently blocking Chrome OS from being able to adopt Dwarf v5, so we are hoping for a fix?

Hmm, apologies. I will take a look tomorrow to try and pick this back up. But unfortunately I'll be away on vacation after tomorrow and it may not be addressed soon. Is Dwarfv5 for ChromeOS bound to a particular llvm release or release date?

Ping? Is anybody looking at or working on this? The bug this fixes is one of the issues currently blocking Chrome OS from being able to adopt Dwarf v5, so we are hoping for a fix?

Committed in r360380.

Test case has been committed, as of revision 357409.

Ran clang-format on Statistics.cpp

It appears to me that on line 131 we are already adding/counting the ConstantMembers in FnStats.VarsInFunction, so adding "+ Constants" would be counting them twice. Alternatively we can change the code on line 131 to not include ConstantMembers...

I was mistaken before: we were never counting the template parameters. What was happening was that we were counting the variables in both inlined instances, and we were also counting the variables for a concrete, out-of-line instance of the inlined function, even though in this particular case there was no concrete out-of-line instance. I have updated the patch now to only count vars in DW_TAG_subprogram DIEs if the subprogram DIE has PC addresses. I believe this is the correct fix for this issue.

This increases the version number, as requested.

Now we correctly count only real constant members as "constant members" and we count global variables with locations as part of the "vars with locs".

aprantl@, you were right, my original change was not correctly updating the variable counts to include variables occuring in inlined instances of functions. I have update the patch to fix this issue. There were two additional changes needed: one to make sure we were counting variables for functions that had no inlined instances at all; and one to avoid double-counting member constants.

Use Conf.CodeModel in LTO, if it exists.

Add comment explaining why mixing two code models is an error. Add missing Inputs/codemodel-3.ll file.

Move include statement from CodeGenModule.h to CodeGenModule.cpp
Remove incorrect "default" case statement.
Add test for "tiny" code model.

Simplify test case, as requested by Peter.

I just realized that my patch did not contain the actual move of RuntimeLibcalls.def from CodeGen to IR. This update includes that move.

Made requested fixes.

I've made Peter's suggested changes.

pcc's original suggested fix did not work, because the real issue was the function definitions being initially internalized by LTO and later DCE'd. Peter's suggestion did not prevent them from being internalized. I consulted with him offline and we came up with this alternative solution. It fixes the issue for the new LTO implemention. The old LTO implementation still internalizes the function definitions, but they do not get Dead Code Eliminated. I have also fixed the test case, and have generalized the code to check for all builtin functions (as requested).

pcc@ Originally I tried to make the change in ModuleSummaryAnalysis.cpp, but it turns out that this is not allowed in LLVM: I am not supposed to make changes to the IR in the Analysis layer of LLVM. I need to find some place in the Transform layer to make the change. Hence my first guess as to a location: GlobalOpt.cpp. I would be happy to make the change in some other location instead, but it must be someplace in the Transform layer.

Make cfi_blacklist.txt be an empty file.

Make -resource-dir point to correct directory, in test case; move cfi_blacklist.txt file to 'share' subdirectory in test resource dir.

I'm not sure if I have commit access or not; Peter was working with me on trying to commit the change.

Fix test failure that my previous changes caused.

vsk: Are you asking me to put together a cfi blacklist to ship in the resource directory in the default install as part of this code review? Or is that something you want to see in a different code reivew?

Added comment to change in source code.

Updated the error to only occur for CFI blacklist, and added test case.

Ok, I'll work on making this CFI-specific and adding a test case.

Tried to upload the diff from the correct location.