CodeView layering

I’m looking at fixing some layering violations in LLVM & came across a few in the CodeView handling, specifically:

lib/MC/MCCodeView includes several llvm/DebugInfo/CodeView headers
I guess MC could be made dependent on DebugInfoCodeView? But probably these things should be sunk into BinaryFormat as is the case for DWARF features used by MC?

include/llvm/Object/COFF.h includes include/llvm/DebugInfo/CodeView/CVDebugRecord.h
Also seems like this could/should/needs to be sunk into BinaryFormat?

I’m open to ideas & happy to do the work, or help in any way that might be useful.

Thanks,

  • Dave

Yes, some of the headers and stuff that are just raw structure definitions and enums could probably be sunk into BinaryFormat…

How’d you find this? Curious why it hasn’t been breaking in modules builds for a long time.

Someone internally’s been dabbling with turning on some of Google’s build system’s stricter header checking modes - and they caught these (you can check the internal code review 184305506 for the original where I’m pulling things out from).

& yeah, fair question about the modules builds - I don’t fully understand what they catch and don’t catch. I suspect it’s a matter of whether the headers themselves form a cycle - whereas the Google build system’s a bit more structured - knowing about libraries and explicit dependencies between them.

& I didn’t look closely to see if, as I said, MC could depend on DebugInfoCodeView - probably weird/awkward/not what we want (I’m guessing?) but the modules buildbots don’t have explicit dependencies, they just fail when they find a cycle. So they can implicitly support all sorts of weird dependencies we might not’ve expected/planned, so long as it’s not an actual cycle (& only within the headers - so the modules system might be like “oh, sure, MC can depend on DebugInfoCodeView” but the library dependency might be the exact opposite (some MC .cpp file could call into DebugInfoCodeView & the modules build wouldn’t fail - it knows nothing about .cpp modularity/grouping, just headers))

Intuitively I’d think MC and DebugInfo should be independent. MC is a producer, DebugInfo is a consumer. What’s common is the definition of the structures they operate on, which doesn’t properly belong to either one.

–paulr

DebugInfoCodeView is nto strictly a consumer, it is also a producer.

Note the same is true of DebugInfoMSF, and DebugInfoPDB (all of which are related).

I’m looking at fixing some layering violations in LLVM & came across a few in the CodeView handling, specifically:

lib/MC/MCCodeView includes several llvm/DebugInfo/CodeView headers
I guess MC could be made dependent on DebugInfoCodeView? But probably these things should be sunk into BinaryFormat as is the case for DWARF features used by MC?

I’d be OK with that. We could very easily introduce true link dependencies on that library in the near future.

include/llvm/Object/COFF.h includes include/llvm/DebugInfo/CodeView/CVDebugRecord.h
Also seems like this could/should/needs to be sunk into BinaryFormat?

These things are really more PE/PDB related than CodeView. They are in the wrong place. I’d ask Saleem to help find a new home. Object or BinaryFormats seem OK to me.

I’m looking at fixing some layering violations in LLVM & came across a few in the CodeView handling, specifically:

lib/MC/MCCodeView includes several llvm/DebugInfo/CodeView headers
I guess MC could be made dependent on DebugInfoCodeView? But probably these things should be sunk into BinaryFormat as is the case for DWARF features used by MC?

I’d be OK with that. We could very easily introduce true link dependencies on that library in the near future.

OK - added the LLVMBuild.txt change in r328595

include/llvm/Object/COFF.h includes include/llvm/DebugInfo/CodeView/CVDebugRecord.h
Also seems like this could/should/needs to be sunk into BinaryFormat?

These things are really more PE/PDB related than CodeView. They are in the wrong place. I’d ask Saleem to help find a new home. Object or BinaryFormats seem OK to me.

Moved it to libObject (r328593) for now which is at least correct - if someone thinks it’d be more suited to BinaryFormat, I’m happy to move it or they can.

I’m looking at fixing some layering violations in LLVM & came across a few in the CodeView handling, specifically:

lib/MC/MCCodeView includes several llvm/DebugInfo/CodeView headers
I guess MC could be made dependent on DebugInfoCodeView? But probably these things should be sunk into BinaryFormat as is the case for DWARF features used by MC?

I’d be OK with that. We could very easily introduce true link dependencies on that library in the near future.

OK - added the LLVMBuild.txt change in r328595

Hmm, not perfect - this creates a bit of a circular dependency:

llvm-tblgen
MC
DebugInfo/CodeView
Object
Bitcode/Reader
IR (including lib/IR/AttributesCompatFunc.inc)
llvm-tblgen (to generate AttributesCompatFunc.inc)

This is just a specific path shown by the internal build system - no doubt there are other tblgen files in LLVM IR, etc.

So I guess still need to split up DebugInfo/CodeView (assuming the bits needed by MC don’t depend on Object)? Any suggestions? Sink it into BinaryFormat or name a new library?

(it’s quite a few headers by the looks of it: CodeView.h, Line.h, SymbolRecord.h, CodeViewTypes.def, CodeViewSymbols.def, CodeViewRegisters.def, TypeIndex.h, CVRecord.h, CodeViewError.h, RecordSerialization.h - maybe some of those aren’t needed, if they’re excessive includes or the like (they tend to seep in all over the place))

It seems a little strange conceptually that object depends on BitcodeReader. Is it possible to break that dependency?

No, Object is supposed to be an abstraction over real object files and LLVM bitcode object files.

Maybe we can break the CodeView → Object dependency.

Looks like maybe the CodeView → Object dependency is out of date/old/not needed any more anyway… (don’t see any Object headers included from the CodeView headers or implementation, etc). Will see if going that way internally is viable & loop back if it stumbles across something.