Page MenuHomePhabricator

[demangler] Fix demangling of 'half'
ClosedPublic

Authored by stuart on Jun 7 2021, 11:39 AM.

Details

Summary

Demangle 'Dh' as 'half' (as per GCC), and not 'decimal16' (which doesn't make sense, as there is no IEEE 754 decimal16 format).

The Itanium C++ ABI specification describes 'Dh' as "IEEE 754r half-precision floating point (16 bits)".

Diff Detail

Unit TestsFailed

TimeTest
620 mslibcxx CI GCC-next/C++20 > libc++.std/atomics/atomics_types_operations/atomics_types_operations_req::atomic_is_lock_free.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/std/atomics/atomics.types.operations/atomics.types.operations.req/atomic_is_lock_free.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/std/atomics/atomics.types.
1,760 mslibcxx CI GCC-next/C++20 > libc++.std/iterators/iterator_primitives/range_iter_ops/range_iter_ops_advance::advance.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.advance/advance.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/std/iterators/iterator.
1,290 mslibcxx CI GCC-next/C++20 > libc++.std/iterators/iterator_primitives/range_iter_ops/range_iter_ops_next::iterator_count.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_count.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/std/iterators/iterator.
1,240 mslibcxx CI GCC-next/C++20 > libc++.std/iterators/iterator_primitives/range_iter_ops/range_iter_ops_next::iterator_count_sentinel.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_count_sentinel.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/std/iterators/iterator.
1,540 mslibcxx CI GCC-next/C++20 > libc++.std/iterators/iterator_primitives/range_iter_ops/range_iter_ops_next::iterator_sentinel.pass.cpp
Script: -- : 'COMPILED WITH'; /usr/bin/g++-11 /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/std/iterators/iterator.primitives/range.iter.ops/range.iter.ops.next/iterator_sentinel.pass.cpp -v -include /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/support/nasty_macros.h -nostdinc++ -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/include/c++/v1 -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/include/c++build -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__STDC_CONSTANT_MACROS -I/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/libcxx/test/support -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-user-defined-literals -Wno-noexcept-type -Wno-aligned-allocation-unavailable -Wno-atomic-alignment -Wno-sized-deallocation -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_DISABLE_AVAILABILITY -Wno-macro-redefined -D_LIBCPP_HAS_THREAD_API_PTHREAD -Wno-macro-redefined -D_LIBCPP_ABI_VERSION=1 -lc++experimental -L/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -L/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -Wl,-rpath,/home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/./lib -nodefaultlibs -lc++ -lm -lgcc_s -lgcc -lpthread -lc -lgcc_s -lgcc -latomic -o /home/libcxx-builder/.buildkite-agent/builds/35a0f1d88717-1/llvm-project/libcxx-ci/build/generic-gcc-next/projects/libcxx/test/std/iterators/iterator.
View Full Test Results (13 Failed)

Event Timeline

stuart requested review of this revision.Jun 7 2021, 11:39 AM
stuart created this revision.
stuart updated this revision to Diff 350407.Jun 7 2021, 1:42 PM
stuart edited the summary of this revision. (Show Details)
stuart added a comment.Jun 7 2021, 1:44 PM
This comment was removed by stuart.
stuart added a comment.Jun 7 2021, 2:03 PM

The pre-merge check failures relating to the failure to include "ItaniumDemangle.h" seem confusing to me. I cannot see how they would be caused by this change, itself: this looks on the surface to be a problem with the CI. Do I need to do anything about this?

The pre-merge check failures relating to the failure to include "ItaniumDemangle.h" seem confusing to me. I cannot see how they would be caused by this change, itself: this looks on the surface to be a problem with the CI. Do I need to do anything about this?

I think this may be because you uploaded a diff without the full path or something...it somehow thinks that you renamed llvm/include/llvm/Demangle/llvm/ItaniumDemangle.h to include/llvm/Demangle/llvm/ItaniumDemangle.h. In "Download Raw Diff" it has, e.g.

--- a/llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ b/include/llvm/Demangle/ItaniumDemangle.h

If you fix the paths in the diff, I think that'll get rid of the spurious "reformat" comments on the unchanged lines, as well.

For the actual change: using the name half here sounds fine, though.

stuart updated this revision to Diff 351100.Jun 10 2021, 2:59 AM

Reupload with no changes.

stuart added subscribers: klimek, chandlerc.EditedJun 10 2021, 3:18 AM

@jyknight, I have reuploaded the exact same diff with full context:

diff --git libcxxabi/src/demangle/ItaniumDemangle.h libcxxabi/src/demangle/ItaniumDemangle.h
--- libcxxabi/src/demangle/ItaniumDemangle.h
+++ libcxxabi/src/demangle/ItaniumDemangle.h
@@ -1,5713 +1,5713 @@
[...]
diff --git llvm/include/llvm/Demangle/ItaniumDemangle.h llvm/include/llvm/Demangle/ItaniumDemangle.h
index 4e3af0cb2649..78035c38ee50 100644
--- llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ llvm/include/llvm/Demangle/ItaniumDemangle.h
@@ -1,5713 +1,5713 @@
[...]

"Download raw diff" still downloads an adjusted version of the diff with the context removed, and with the paths mangled (ironically) by Differential.

The full context is visible in Differential's pretty-printing of the diff. I believe the context would not be visible had I not included it in the patch as submitted.

Perhaps the path of "llvm/include/llvm" is confusing it. This change does apply to both libcxxapi and llvm, but I would prefer not to split it up if there is no need.

@klimek, @chandlerc, would you have any advice? I've read the Code Reviews with Phabricator guide (and also the LLVM Developer Policy) and can't see anything there that would indicate that I'm doing anything wrong.

Update: I've tried uploading again with "llvm-project" added to both the "a" and "b" paths, with the result that "[ab]/llvm-project/llvm" and "[ab]/llvm-project/libcxxabi" are now getting stripped out instead.

Please help.

stuart updated this revision to Diff 351117.Jun 10 2021, 3:44 AM

Reupload with no changes.

This comment was removed by stuart.

A git diff generally has an extraneous path component [ab] first, e.g. your diff should look like this:

diff --git a/libcxxabi/src/demangle/ItaniumDemangle.h b/libcxxabi/src/demangle/ItaniumDemangle.h
index 4e3af0cb2649..78035c38ee50 100644
--- a/llvm/include/llvm/Demangle/ItaniumDemangle.h
+++ b/llvm/include/llvm/Demangle/ItaniumDemangle.h

but your diff excerpt in the comment above is missing that extra directory prefix. I suspect you've got a local configuration file setting diff.noprefix = true -- and unfortunately, while omitting the a/b prefixes is nicer for humans, it confuses most git tooling, which expects it to be present -- including git itself, e.g. git apply.

stuart updated this revision to Diff 351153.Jun 10 2021, 6:23 AM

Add a/ and b/ prefixes to diff.

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 10 2021, 6:23 AM

but your diff excerpt in the comment above is missing that extra directory prefix. I suspect you've got a local configuration file setting diff.noprefix = true

Good catch. I've fixed this.

I've long since forgotten why I ever added diff.noprefix = true to my ~/.gitconfig... I'm not sure it was humans, actually! If I ever need this again, I'll make sure I add it as a local setting for the repositories where I need it! :-)

Sorry, @klimek and @chandlerc, for the noise.

jyknight accepted this revision.Jun 10 2021, 9:03 AM

(I note the new behavior also matches gcc)

(I note the new behavior also matches gcc)

Good point. (Also, how I came to be aware of this, and what actually informed the choice of "half" rather than "binary16", for that matter.)

Would you agree that it makes sense to mention this in the commit message?

stuart edited the summary of this revision. (Show Details)Jun 10 2021, 12:39 PM
stuart updated this revision to Diff 354107.Jun 23 2021, 4:32 PM

No chances since last time. Rebased to force a rebuild.

If I understand correctly, this needs review from the libc++abi group before I can push it. It's a fairly trivial change. Could someone review this please? (Is there anyone specifically I should add?)

stuart edited the summary of this revision. (Show Details)Jul 19 2021, 7:04 AM
stuart edited the summary of this revision. (Show Details)
ldionne accepted this revision.Jul 19 2021, 12:08 PM
This revision is now accepted and ready to land.Jul 19 2021, 12:08 PM
This revision was automatically updated to reflect the committed changes.