I think we can reduce the test even more, but otherwise this should be fine.

Seems reasonable to me.

Thanks for the patch! The testcase looks like it would bitrot very easily. I would recommend first manually reducing the IR as much as possible and then making a MIR testcase out of it to become independent from future ISEL changes.

Looks like this test is failing on green dragon:

I changed all the autos back to full types in r351237.

Thanks Pavel, your argument of optimizing storage over interfaces makes perfect sense. Using an Optional instead.

I didn't realize you were talking about DwarfExpression, sorry . I'm not really sure what the best solution is because I don't quite understand MCLabels that well. Naively I was assuming that we would emit an MCLabel that is defined as the difference between DIE offset and .debug_info start label. Since we need to emit DIE references all over .debug_info I would start by looking at how it's done there. The fact that it is in a different section may complicate things though, I'm not sure.

Roughly: the implementation of print used code (MachineInstr operator<<) that lives in the CodeGen library, and various tools that don't link against CodeGen include header files from the CodeGen Clang module). Without local submodule visibility, this pulls in all headers of the CodeGen Clang module, thus causing code to be generated for the dump function, which then causes the linker error.

Seems okay to me, but it would be good to wait Björn's LGTM.

In D56587#1356420, @markus wrote:

In D56587#1356258, @aprantl wrote:

Referencing a DIE in DIExpression would need a bit of additional work. DIExpression stores its operands in an array of uint64_t, so in order to support MDNode operands we'd have to add them as actual MDNode operands. For example, expr_op_iterator could know which DIExpression operations take MDNode arguments and inject them in the right places. Slightly more complicated would be actually finding a matching DIType in the debug info that we want to point to. If we want to convert to exactly the type of the DIVariable, we can use that type, otherwise we might have to generate new DIBasicTypes on the fly.

Even though this sounds complicated, I still think that it is preferable to encode type conversions as such rather than generating really complicated expressions that happen to have the same effect in the underspecified DWARF 4 stack language.

Yes, long term it is likely the best solution.

I played a bit with trying to insert a DW_OP_convert before I came up with this patch but was clueless on how to retrieve the DIE offset of the DIType when the expression was emitted as that section (.debug_info?) hadn't been emitted yet. If I could get some useful advice on tackling that issue I can have another go at it when I get back to work tomorrow.

I *think* I fixed it for good in in r351077.

In D56587#1355882, @markus wrote:

Probably inserting a pseudo op here lowering it at a later stage when it is known if the value will reside in memory would be the right thing to do. Not sure if a DWARF 5 DW_OP_convert would be the easiest option as its argument references another DIE and it seems that would require larger infrastructure changes (but I really don't know anything about this).

ii) It is probably not safe to assume that the consumer/debugger would set the high bits to zero in zero extension in e.g. the case that the variable has been spilled to memory.

You have to use clang in order to compile with clang module support enabled. If your host compiler is GCC, you will have to build clang first and then use that clang to compile clang again using all the MODULE cmake options.

It should not be mac-specific, but it is possible that Linux uses a different default for LLVM_ENABLE_LOCAL_SUBMODULE_VISIBILITY. Can you try again with -DLLVM_ENABLE_LOCAL_SUBMODULE_VISIBILITY=0 -DLLVM_ENABLE_MODULES=1 ?

Hmmm.. my fix didn't go far enough. I reverted this patch once again, so we can sort this out without pressure. As long as you make sure that compiling with -DLLVM_ENABLE_MODULES=1 works, feel free to recommit at any time!

I committed a rather horrible workaround in r350650, but it would be great to revert that again.

There's a type in a test, but otherwise, this LGTM.

Looks like these are unrelated failures. TestQueues started acting up last week because of a concurrency issue. I would recommend you attempt to re-land your commit again and see if any new failures show up. If you are unsure, feel free to ask here or on #llvm on IRC.

In D56084#1347564, @lsaba wrote:

In D56084#1346142, @sgraenitz wrote:

@aprantl Found a workaround for the lldb-cmake bot: https://reviews.llvm.org/rL350346

I tested it locally with your patch reapplied and the error is fixed. Sorry again for the hassle. Please submit your patch. Thanks

@aprantl , @sgraenitz , I tried to resbumit the patch in https://reviews.llvm.org/rL350493 but unfortunately issues with the modules still appear: http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/314/steps/test/logs/stdio

it is now able to build but the lit test SymbolFile/PDB/class-layout.test is failing:

$ ":" "RUN: at line 4"
$ "E:\build_slave\lldb-x64-windows-ninja\build\bin\lldb-test.EXE" "symbols" "E:\build_slave\lldb-x64-windows-ninja\build\tools\lldb\lit\SymbolFile\PDB\Output/ClassLayoutTest.cpp.exe"

1. command stderr: error: Module has no symbol vendor.

error: command failed with exit status: 1
$ "E:\build_slave\lldb-x64-windows-ninja\build\bin\FileCheck.EXE" "E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\lit\SymbolFile\PDB\class-layout.test"

1. command stderr: E:\build_slave\lldb-x64-windows-ninja\llvm\tools\lldb\lit\SymbolFile\PDB\class-layout.test:15:8: error: CHECK: expected string not found in input CHECK: Module [[MOD:.*]] ^ <stdin>:1:1: note: scanning from here Module: E:\build_slave\lldb-x64-windows-ninja\build\tools\lldb\lit\SymbolFile\PDB\Output/ClassLayoutTest.cpp.exe

I have reverted the patch again for now as I am unable to reproduce the issue locally, your help will be much apprecicated

In D56297#1346801, @andrewrk wrote:

In D56297#1345884, @aprantl wrote:

Is the problem that it is a SubroutineType, but not a function *pointer*?
Generally, if the Verifier accepts it we ought to not crash, so this seems to be okay, but it might be worth asking whether this is really what you want your frontend to be creating since subroutinetypes don't have a size, etc.

I'm the author of the frontend that is generating this code. Thanks for the explanation of the problem. Based on this I tried similar code with clang:

Is the problem that it is a SubroutineType, but not a function *pointer*?
Generally, if the Verifier accepts it we ought to not crash, so this seems to be okay, but it might be worth asking whether this is really what you want your frontend to be creating since subroutinetypes don't have a size, etc.

A testcase that triggers this situation would be nice, too.

I implemented a poor man's version of that in r350360.

Looks generally good to me.

In D52634#1335761, @dschuff wrote:

@aprantl Is the advantage of your suggested approach just that we don't have to define a new expression type? Obviously the interpretation is not the same as DW_OP_breg on other targets so as you say, either way there would have to be special logic in all the tools that consume it. Is this kind of repurposing of builtin primitives common?

Sorry I forgot to comment: this should be fixed in r349533.

Ah.. that's possible. REQUIRES needs to be very high up in the file, but I don't know what the exact threshold is

I also found that the tools (e.g. LLVM or LLDB) are eager to know about number of registers (which we cannot determine) to allocate internal structures.

In D52634#1334438, @yurydelendik wrote:

In D52634#1293526, @aprantl wrote:

Can you explain how this new operation is meant to work?

@aprantl, here is some description of what we are trying to achieve: https://github.com/WebAssembly/debugging/issues/1#issuecomment-448237641

In the nutshell, the WebAssembly extensions for the DWARF expression will allow us to express WebAssembly specific locations such as for locals, globals and operands stack. The consumers of this information will be limited to LLVM tools, web browser debuggers and AOT/JIT WebAssembly compilers.

Can you confirm that reverting this path actually fixes the issue? I'm asking because the only test that is executing this script has a REQUIRES: system-darwin line in it.

Got rid of the no_uuid regex and added even more testcases.

Fixed the reported problems with the regexes and added testcases with example lines. I used the funny test setup to avoid burdening the installed crashlog.py script with unit test code that would need to be parsed every time.

Looks like this caused a noticeable shrinking of location lists, but that means we're getting more accurate: http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607042.4&highlight_run=117379 !

Can you watch http://lnt.llvm.org/db_default/v4/nts/117379 (http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607042.4 and http://lnt.llvm.org/db_default/v4/nts/graph?plot.0=1357.1607043.4) after landing this?

Please be sure to watch all LLDB bots (green dragon and lab.llvm.org:8011) closely when landing this change.

Let's give this a try.

I believe I fixed this in r349065. (The DIFile used by the CU is special and distinct from the main source file. Its directory part specifies what becomes the DW_AT_comp_dir (the compilation directory), even if the source file was specified with an absolute path.)

Be more verbose about what is a valid arch.

This one is probably less controversial than D55574 :-)

I'm mostly on board with making these changes, as it's good LLVM style to do this. I highlighted a couple changes that might warrant a closer look.

I just verified that this is no longer applicable as there is no longer a DW_OP_deref in the inlined function.

Looks like this landed in r345274 and wasn't properly closed.

I also removed the other references to DIBuilder::createFile() in r348866.

LGTM with outstanding comment addressed.

Thanks!

Thanks, should be fixed in r348610!

LGTM if @vsk's question is answered satisfactorily.