[PATCH] D27551: [DebugInfo] Add regression test for __fp16, float, and double constants.

[David Blaikie via llvm-commits]

Ah, I see (from your sample, and the test case here) - we used to use a
ConstantFP in the IR, but now we just provide the raw bytes so it's a
non-issue.

Yeah, I might've committed a test if this change had intended to address
that - but now that it's there/a non-issue by design without any code
needed to test it, I'm ambivalent/might not bother with it myself. *shrug*
:)

On Mon, Dec 12, 2016 at 4:32 PM David Gross <dgross at google.com> wrote:

> On Mon, Dec 12, 2016 at 4:15 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Dec 12, 2016 at 4:11 PM David Gross <dgross at google.com> wrote:
>
> Per the Details:
>
> Prior to the change "DebugInfo: New metadata representation for global
>
> variables." (D20147 <https://reviews.llvm.org/D20147>, D20415
> <https://reviews.llvm.org/D20415>), clang emitted debug metadata for
> static
>
> const values of floating-point types, but llvm did not translate this
> debug metadata into DWARF. After that change, clang did not emit debug
> metadata for static const values of floating-point types, but llvm is
> capable of translating it into DWARF if it is emitted.
>
>
> My (perhaps overly cautious) concern is that given what has occurred in
> the past (where llvm was not emitting bytes for floating-point types), it's
> possible that there might be a change in the future which would cause the
> emission of the bytes to become type-sensitive again, and I'd like to be
> sure we do continue to emit the bytes for floating-point types.
>
>
> Hmm - could you point me to the particular parts of those changes (or
> roughly describe what was going on in the old code, and what goes on in the
> new code) that caused this change in behavior?
>
>
>
> Briefly, in the old version of clang/llvm, clang emits debug metadata
> that's at a higher semantic level (that is, it's not emitting a
> DIExpression), and that debug metadata is type-independent (or at least
> treats integral types and floating-point types the same); but when llvm
> translates that debug metadata into DWARF, it does not do so for
> floating-point types.
>
> (Looking back on what I wrote, I realize I just restated the Details from
> this patch.)  You could look at the two changes cited in the Details where
> the metadata representation changed.
>
> Also, here is a patch applied to the old version of clang/llvm to make a
> similar fix.  Whereas for current versions of clang/llvm, the fix is to
> modify clang to emit debug metadata for floating-point types (llvm already
> does the right thing), for the old version of clang/llvm shown in the
> following patches, the fix is to modify llvm to emit DWARF for
> floating-point types (clang already does the right thing):
> https://android-review.googlesource.com/#/c/312566/1/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
>
>
>
> - Dave
>
>
>
>
> On Mon, Dec 12, 2016 at 3:47 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Dec 12, 2016 at 3:24 PM David Gross via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> dgross added a comment.
>
> In https://reviews.llvm.org/D27551#620373, @dblaikie wrote:
>
> > Does this need the test coverage if it's just "hey, LLVM can splat these
> bytes out into DWARF even if we (independently) say the type of those bytes
> is a float"? The code that emits the bytes is presumably independent of the
> code that emits the type description, so testing the combination seems
> unnecessary to me.
>
>
> Can you explain this further?  What parts of this test case are you
> suggesting may be unnecessary?
>
>
> All of it. (partly under the argument that no code changes were required
> to provide this functionality, but to be more complete:
>
> * We already have test coverage for emitting the correct DW_AT_type value
> here
> * We already have test coverage that the arbitrary bytes in the constant
> value are emitted into the right place in the DW_AT_const_value or whatnot
>
> So what's the particularly interesting thing about testing the combination
> of those two (so far as I can see) orthogonal features?)
>
>
>
>
> https://reviews.llvm.org/D27551
>
>
>
>
>
>
> Normal
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161213/b0d1d4b8/attachment-0001.html>