New user self-registration is disabled due to spam. For an account please email bugs-admin@lists.llvm.org with your e-mail address and full name.

Bug 47287 - codeview assertion fails on lowerBound
Summary: codeview assertion fails on lowerBound
Status: RESOLVED FIXED
Alias: None
Product: libraries
Classification: Unclassified
Component: Common Code Generator Code (show other bugs)
Version: 11.0
Hardware: PC Windows NT
: P enhancement
Assignee: Unassigned LLVM Bugs
URL:
Keywords:
Depends on:
Blocks: release-11.0.0
  Show dependency tree
 
Reported: 2020-08-22 10:37 PDT by Josh Stone
Modified: 2020-09-11 02:47 PDT (History)
7 users (show)

See Also:
Fixed By Commit(s):


Attachments
IR for the unboxed-closures-unique-type-id.rs test (112.58 KB, text/plain)
2020-08-22 10:37 PDT, Josh Stone
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Josh Stone 2020-08-22 10:37:11 PDT
Created attachment 23885 [details]
IR for the unboxed-closures-unique-type-id.rs test

We encountered this assertion in Rust x86_64-msvc trying to upgrade to LLVM 11.
https://github.com/rust-lang/rust/pull/73526#issuecomment-678576555

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1595   │     assert(!Subrange->getRawLowerBound() &&
1596   │            "codeview doesn't support subranges with lower bounds");


The IR for that failed test is attached. It contains this:

    !157 = !DISubrange(count: 3, lowerBound: 0)

The actual assertion was changed in D80197, specifically here:
https://github.com/llvm/llvm-project/commit/d20bf5a7258d4b6a7f017a81b125275dac1aa166#diff-8412e58a7e99d6349b9f9418af573792

I confirmed that our IR passes the assertion before that commit, when it checked "Subrange->getLowerBound() == 0". I think that "!Subrange->getRawLowerBound()" is not equivalent though because that raw return value is a "Metadata *", so the assertion is now checking that it's NULL. This is different than having the lowerBound present with a constant value 0.
Comment 1 Alok Kumar Sharma 2020-08-23 06:28:20 PDT
With the commit for DISubrange upgrade, there is change in supported types for lowerBound field of DISubrange and its internal representation.
Earlier it supported integer type and its internal representation was integer.
  internal representation with zero was representing two things
    - lowerBound is absent (like DISubrange(count 3)
  Or, lowerBound is present with value zero (DISubrange(count: 3, lowerBound: 0)

With upgrade to DISubrange,
  there are different internal representations for above two different cases.

Now coming to current code line.

llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp
1595   │     assert(!Subrange->getRawLowerBound() &&
1596   │            "codeview doesn't support subranges with lower bounds");

the assertion comment "codeview doesn't support subranges with lower bounds", suggests it expects absent lowerBound, this is why new condition is so.

Should not the failed IR be actually invalidated as per the assertion comment, the way it is happening now?

In case it lowerBound present with value zero is valid IR for Codeview then we must also update the assertion comment to reflect this along with the condition. Please let me know your view.
Comment 2 Josh Stone 2020-08-24 10:24:43 PDT
I don't really know anything about codeview, but my impression is that "doesn't support lower bounds" really means that the lower bound is always implicitly zero, because the format doesn't have a way to specify anything else. If so, then it shouldn't be an error to have an explicit zero either.

Rust is using DIBuilder::getOrCreateSubrange(int64_t Lo, int64_t Count) -- and before your change the other choice was only (int64_t Lo, Metadata *CountNode). I suppose now we could use your new one with 4 Metadata*, filling only the count and leaving the others NULL. But first I think we should get a codeview person to answer whether lowerBound:0 ought to be accepted here.
Comment 3 Hans Wennborg 2020-08-24 12:53:28 PDT
+some folks who can maybe help with the codeview angle
Comment 4 Amy Huang 2020-08-24 18:32:00 PDT
My impression is that the codeview code checked for `== 0` because the IR used to use lowerBound:0 to denote no lower bound.

Based on Alok's comment we now use a null metadata to represent no lower bound, so using the new getOrCreateDISubrange seems like the right thing to do if there is no explicit lower bound.
Comment 5 Hans Wennborg 2020-08-25 05:17:28 PDT
Okay, sounds like there's no bug here then, but that the API has changed.

Alok, could you send a patch to llvm/docs/ReleaseNotes.rst on the release/11.x branch that mentions this change? I think the "Changes to the LLVM IR" section would be a good spot.
Comment 6 Josh Stone 2020-08-25 11:40:37 PDT
All of the non-empty examples of DISubrange in the LangRef have a lowerBound, with a comment on the first, "array counting from 0". It seems strange to suggest that you must not specify any lowerBound for codeview.

(In reply to Amy Huang from comment #4)
> My impression is that the codeview code checked for `== 0` because the IR
> used to use lowerBound:0 to denote no lower bound.

I don't know what "no lower bound" would really *mean*. An indexable array should always have a first index, no? That may be an implied 0, and maybe codeview can't express anything else, but ISTM the concept always exists.
Comment 7 Josh Stone 2020-08-25 12:15:05 PDT
For comparison, the DWARF specification does allow DW_TAG_subrange_type to omit DW_AT_lower_bound, but then it assumes a default lower bound according to the current DW_AT_language -- table 7.17 in DWARF5.
Comment 8 Amy Huang 2020-08-25 14:12:32 PDT
Oh, I see. That makes sense, I agree that lower bound zero should also be ok for codeview.
Comment 9 Hans Wennborg 2020-08-26 08:00:07 PDT
Alok, can you help fix this?
Comment 10 Hans Wennborg 2020-09-07 11:17:34 PDT
(In reply to Hans Wennborg from comment #9)
> Alok, can you help fix this?

Adrian, you reviewed the patch above. Any ideas?
Comment 11 Alok Kumar Sharma 2020-09-09 04:22:42 PDT
Okey, I shall push a patch to update the condition to accept "lowerBound: 0". I shall update the message as well.
Comment 12 Alok Kumar Sharma 2020-09-09 04:30:33 PDT
(In reply to Alok Kumar Sharma from comment #11)
> Okey, I shall push a patch to update the condition to accept "lowerBound:
> 0". I shall update the message as well.

Since now, lowerBound also supports Metadata and there can be 0 value (which is detectable at runtime). Should we not remove this assertion in place of updating it. Any suggestion ?
Comment 13 Alok Kumar Sharma 2020-09-09 11:47:44 PDT
Please have a look at 
https://reviews.llvm.org/D87406
Comment 14 Hans Wennborg 2020-09-11 02:47:24 PDT
(In reply to Alok Kumar Sharma from comment #13)
> Please have a look at 
> https://reviews.llvm.org/D87406

That was committed in e45b0708ae81 and merged to 11.x in 29d700a8132088ee6320702b601c0479a710a3ec

Thanks!