[PATCH] D148042: [MSP430] Get the DWARF pointer size from MCAsmInfo instead of DataLayout.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Not sure these elements are sufficiently well documented/specified to make this super reliable in terms of the chances this property will regress in the future - but at least eliminating all the calls as you've done here gives a better chance that someone'll copy from an existing use case rather than introducing a novel one.



================
Comment at: llvm/lib/Target/MSP430/MCTargetDesc/MSP430MCAsmInfo.cpp:19-21
+  // LLDB assumes that the pointer size in DWARF information is the same as in
+  // the ELF32 header, so generating 16-bit DWARF pointers within ELF32 causes
+  // problems.
----------------
I wouldn't justify this from a "what LLDB assumes" - again, we could "just" fix LLDB if that's the problem.


================
Comment at: llvm/lib/Target/MSP430/MCTargetDesc/MSP430MCAsmInfo.cpp:23-24
+  // Since MSP430-GCC already generates 32-bit DWARF information, we will
+  // also store 16-bit pointers as 32-bit pointers in DWARF in order to
+  // maintain compatibility with GCC binaries and leave LLDB's pointer
+  // assumption as is.
----------------
It's not really compatibility with GCC binaries though - the DWARF could be different between the two compilers and that could be supported by consumers.

Maybe the simplest wording would be something like "Since this is what GCC does, we'll do it to since it's likely a better/well tested path for DWARF consumers like GDB and LLDB, rather than something that might be novel though possibly more technically correct"?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148042/new/

https://reviews.llvm.org/D148042