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 -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180321/0fbfe1dc/attachment.html>
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. On Wed, Mar 21, 2018 at 11:31 AM David Blaikie <dblaikie at gmail.com> wrote:> 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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180321/781d8d44/attachment.html>
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)) On Wed, Mar 21, 2018 at 11:42 AM Zachary Turner <zturner at google.com> wrote:> 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. > > On Wed, Mar 21, 2018 at 11:31 AM David Blaikie <dblaikie at gmail.com> wrote: > >> 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 >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180321/4a29f1f5/attachment.html>
On Wed, Mar 21, 2018 at 11:31 AM David Blaikie <dblaikie at gmail.com> wrote:> 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 open to ideas & happy to do the work, or help in any way that might be > useful. > > Thanks, > - Dave >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180322/69c4c560/attachment.html>
On Thu, Mar 22, 2018 at 12:55 PM Reid Kleckner <rnk at google.com> wrote:> On Wed, Mar 21, 2018 at 11:31 AM David Blaikie <dblaikie at gmail.com> wrote: > >> 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 open to ideas & happy to do the work, or help in any way that might >> be useful. >> >> Thanks, >> - Dave >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180326/8b74b963/attachment.html>