[llvm] r357077 - [BPF] use std::map to ensure consistent output
Yonghong Song via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 27 09:15:41 PDT 2019
On 3/27/19 8:51 AM, Roman Lebedev wrote:
> On Wed, Mar 27, 2019 at 6:43 PM Yonghong Song via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
>>
>> Author: yhs
>> Date: Wed Mar 27 08:45:27 2019
>> New Revision: 357077
>>
>> URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D357077-26view-3Drev&d=DwIFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=9Ed_8fP5kiQopGkmnLK5sLqoEFP7juhZ1lNRaWQgYoU&s=NCE-ajjQoEPEyGp0rcXPLpKRTDfaLDiW2k-iV7DNQHg&e=
>> Log:
>> [BPF] use std::map to ensure consistent output
>>
>> The .BTF.ext FuncInfoTable and LineInfoTable contain
>> information organized per ELF section. Current definition
>> of FuncInfoTable/LineInfoTable is:
>> std::unordered_map<uint32_t, std::vector<BTFFuncInfo>> FuncInfoTable
>> std::unordered_map<uint32_t, std::vector<BTFLineInfo>> LineInfoTable
>> where the key is the section name off in the string table.
>> The unordered_map may cause the order of section output
>> different for different platforms.
>>
>> The same for unordered map definition of
>> std::unordered_map<std::string, std::unique_ptr<BTFKindDataSec>>
>> DataSecEntries
>> where BTF_KIND_DATASEC entries may have different ordering
>> for different platforms.
>>
>> This patch fixed the issue by using std::map.
>> Test static-var-derived-type.ll is modified to generate two
>> DataSec's which will ensure the ordering is the same for all
>> supported platforms.
>>
>> Signed-off-by: Yonghong Song <yhs at fb.com>
>>
>> Modified:
>> llvm/trunk/lib/Target/BPF/BTFDebug.h
>> llvm/trunk/test/CodeGen/BPF/BTF/static-var-derived-type.ll
>>
>> Modified: llvm/trunk/lib/Target/BPF/BTFDebug.h
>> URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Target_BPF_BTFDebug.h-3Frev-3D357077-26r1-3D357076-26r2-3D357077-26view-3Ddiff&d=DwIFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=9Ed_8fP5kiQopGkmnLK5sLqoEFP7juhZ1lNRaWQgYoU&s=cJnZiWkkilP6jEms2gGj5X2FA78DrVr-P8tfrt4h-Gc&e=
>> ==============================================================================
>> --- llvm/trunk/lib/Target/BPF/BTFDebug.h (original)
>> +++ llvm/trunk/lib/Target/BPF/BTFDebug.h Wed Mar 27 08:45:27 2019
>> @@ -229,10 +229,10 @@ class BTFDebug : public DebugHandlerBase
>> BTFStringTable StringTable;
>> std::vector<std::unique_ptr<BTFTypeBase>> TypeEntries;
>
>> std::unordered_map<const DIType *, uint32_t> DIToIdMap;
> It is a really bad idea to use std::unordered_map<>.
> It has horrible perf (and memory) characteristics, and (i may be
> wrong) is recommended to not be used in LLVM.
> Why not DenseMap?
The initial BTF patch https://reviews.llvm.org/D53736 is reviewed.
The use of unordered map is specifically to address review comments:
====
MaskRay added inline comments.
lib/CodeGen/AsmPrinter/BTFDebug.cpp
143
const DINode *
145
cast if you don't think it can be null. LLVM_ENABLE_ASSERTIONS builds
have assert() in cast()
196
cast
491
dyn_cast + assert can just be replaced by cast
lib/CodeGen/AsmPrinter/BTFDebug.h
172
StringRef
200
Must these maps be ordered?
203
Consider StringMap
====
====
Address Fangrui's comments:
using "cast" instead of "dyn_cast" and removed a few assertions.
change argument of addString from std::string to StringRef.
change std::map to std::unordered_map in several instances as we do not
really care its ordering.
Do not really understand the pros/cons of StringMap vs. std::map, but
anyway changed FileContent to use StringMap.
====
>
> i guess it doesn't help that BPF patches aren't being reviewed on the
> llvm-commits..
Any large change (e.g., the new functionality, etc.) other than a simple
bug fix, we do put to llvm-commits. The previous one is
https://reviews.llvm.org/D59441
and this change is a bug fix for the above commit.
We used to do more kernel style code reviews through `git send-email`.
I totally agree with you. Let us put all the bpf patches (except
extremely trivial ones, or urgent revert, etc.) in llvm phabricator. I
hope you will help to review bpf patches in the future.
Thanks!
>
>> - std::unordered_map<uint32_t, std::vector<BTFFuncInfo>> FuncInfoTable;
>> - std::unordered_map<uint32_t, std::vector<BTFLineInfo>> LineInfoTable;
>> + std::map<uint32_t, std::vector<BTFFuncInfo>> FuncInfoTable;
>> + std::map<uint32_t, std::vector<BTFLineInfo>> LineInfoTable;
>> StringMap<std::vector<std::string>> FileContent;
>> - std::unordered_map<std::string, std::unique_ptr<BTFKindDataSec>>
>> + std::map<std::string, std::unique_ptr<BTFKindDataSec>>
>> DataSecEntries;
>>
>> /// Add types to TypeEntries.
>>
>> Modified: llvm/trunk/test/CodeGen/BPF/BTF/static-var-derived-type.ll
>> URL: https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_CodeGen_BPF_BTF_static-2Dvar-2Dderived-2Dtype.ll-3Frev-3D357077-26r1-3D357076-26r2-3D357077-26view-3Ddiff&d=DwIFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=9Ed_8fP5kiQopGkmnLK5sLqoEFP7juhZ1lNRaWQgYoU&s=u43ikNmpQPZiWqMGeZPomuTlbon95b93WiHtbOb-utQ&e=
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/BPF/BTF/static-var-derived-type.ll (original)
>> +++ llvm/trunk/test/CodeGen/BPF/BTF/static-var-derived-type.ll Wed Mar 27 08:45:27 2019
>> @@ -6,26 +6,32 @@
>> ; static int * volatile v1;
>> ; static const int * volatile v2;
>> ; static volatile int_ptr v3 = 0;
>> -; long foo() { return (long)(v1 - v2 + v3); }
>> +; static volatile const int_ptr v4 = 0;
>> +; long foo() { return (long)(v1 - v2 + v3 - v4); }
>> ; Compilation flag:
>> ; clang -target bpf -O2 -g -S -emit-llvm test.c
>>
>> @v1 = internal global i32* null, align 8, !dbg !0
>> @v2 = internal global i32* null, align 8, !dbg !8
>> @v3 = internal global i32* null, align 8, !dbg !14
>> + at v4 = internal constant i32* null, align 8, !dbg !19
>>
>> ; Function Attrs: norecurse nounwind
>> -define dso_local i64 @foo() local_unnamed_addr #0 !dbg !24 {
>> - %1 = load volatile i32*, i32** @v1, align 8, !dbg !26, !tbaa !27
>> - %2 = load volatile i32*, i32** @v2, align 8, !dbg !31, !tbaa !27
>> - %3 = ptrtoint i32* %1 to i64, !dbg !32
>> - %4 = ptrtoint i32* %2 to i64, !dbg !32
>> - %5 = sub i64 %3, %4, !dbg !32
>> - %6 = ashr exact i64 %5, 2, !dbg !32
>> - %7 = load volatile i32*, i32** @v3, align 8, !dbg !33, !tbaa !27
>> - %8 = getelementptr inbounds i32, i32* %7, i64 %6, !dbg !34
>> - %9 = ptrtoint i32* %8 to i64, !dbg !35
>> - ret i64 %9, !dbg !36
>> +define dso_local i64 @foo() local_unnamed_addr #0 !dbg !27 {
>> + %1 = load volatile i32*, i32** @v1, align 8, !dbg !29, !tbaa !30
>> + %2 = load volatile i32*, i32** @v2, align 8, !dbg !34, !tbaa !30
>> + %3 = ptrtoint i32* %1 to i64, !dbg !35
>> + %4 = ptrtoint i32* %2 to i64, !dbg !35
>> + %5 = sub i64 %3, %4, !dbg !35
>> + %6 = ashr exact i64 %5, 2, !dbg !35
>> + %7 = load volatile i32*, i32** @v3, align 8, !dbg !36, !tbaa !30
>> + %8 = getelementptr inbounds i32, i32* %7, i64 %6, !dbg !37
>> + %9 = load volatile i32*, i32** @v4, align 8, !dbg !38, !tbaa !30
>> + %10 = ptrtoint i32* %8 to i64, !dbg !39
>> + %11 = ptrtoint i32* %9 to i64, !dbg !39
>> + %12 = sub i64 %10, %11, !dbg !39
>> + %13 = ashr exact i64 %12, 2, !dbg !39
>> + ret i64 %13, !dbg !40
>> }
>>
>> ; CHECK: .section .BTF,"", at progbits
>> @@ -34,9 +40,9 @@ define dso_local i64 @foo() local_unname
>> ; CHECK-NEXT: .byte 0
>> ; CHECK-NEXT: .long 24
>> ; CHECK-NEXT: .long 0
>> -; CHECK-NEXT: .long 236
>> -; CHECK-NEXT: .long 236
>> -; CHECK-NEXT: .long 84
>> +; CHECK-NEXT: .long 288
>> +; CHECK-NEXT: .long 288
>> +; CHECK-NEXT: .long 95
>> ; CHECK-NEXT: .long 0 # BTF_KIND_FUNC_PROTO(id = 1)
>> ; CHECK-NEXT: .long 218103808 # 0xd000000
>> ; CHECK-NEXT: .long 2
>> @@ -84,7 +90,14 @@ define dso_local i64 @foo() local_unname
>> ; CHECK-NEXT: .long 234881024 # 0xe000000
>> ; CHECK-NEXT: .long 12
>> ; CHECK-NEXT: .long 0
>> -; CHECK-NEXT: .long 79 # BTF_KIND_DATASEC(id = 15)
>> +; CHECK-NEXT: .long 0 # BTF_KIND_CONST(id = 15)
>> +; CHECK-NEXT: .long 167772160 # 0xa000000
>> +; CHECK-NEXT: .long 12
>> +; CHECK-NEXT: .long 79 # BTF_KIND_VAR(id = 16)
>> +; CHECK-NEXT: .long 234881024 # 0xe000000
>> +; CHECK-NEXT: .long 15
>> +; CHECK-NEXT: .long 0
>> +; CHECK-NEXT: .long 82 # BTF_KIND_DATASEC(id = 17)
>> ; CHECK-NEXT: .long 251658243 # 0xf000003
>> ; CHECK-NEXT: .long 0
>> ; CHECK-NEXT: .long 7
>> @@ -96,6 +109,12 @@ define dso_local i64 @foo() local_unname
>> ; CHECK-NEXT: .long 14
>> ; CHECK-NEXT: .long v3
>> ; CHECK-NEXT: .long 8
>> +; CHECK-NEXT: .long 87 # BTF_KIND_DATASEC(id = 18)
>> +; CHECK-NEXT: .long 251658241 # 0xf000001
>> +; CHECK-NEXT: .long 0
>> +; CHECK-NEXT: .long 16
>> +; CHECK-NEXT: .long v4
>> +; CHECK-NEXT: .long 8
>> ; CHECK-NEXT: .byte 0 # string offset=0
>> ; CHECK-NEXT: .ascii ".text" # string offset=1
>> ; CHECK-NEXT: .byte 0
>> @@ -115,23 +134,27 @@ define dso_local i64 @foo() local_unname
>> ; CHECK-NEXT: .byte 0
>> ; CHECK-NEXT: .ascii "v3" # string offset=76
>> ; CHECK-NEXT: .byte 0
>> -; CHECK-NEXT: .ascii ".bss" # string offset=79
>> +; CHECK-NEXT: .ascii "v4" # string offset=79
>> +; CHECK-NEXT: .byte 0
>> +; CHECK-NEXT: .ascii ".bss" # string offset=82
>> +; CHECK-NEXT: .byte 0
>> +; CHECK-NEXT: .ascii ".rodata" # string offset=87
>> ; CHECK-NEXT: .byte 0
>>
>> attributes #0 = { norecurse nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "unsafe-fp-math"="false" "use-soft-float"="false" }
>>
>> !llvm.dbg.cu = !{!2}
>> -!llvm.module.flags = !{!20, !21, !22}
>> -!llvm.ident = !{!23}
>> +!llvm.module.flags = !{!23, !24, !25}
>> +!llvm.ident = !{!26}
>>
>> !0 = !DIGlobalVariableExpression(var: !1, expr: !DIExpression())
>> -!1 = distinct !DIGlobalVariable(name: "v1", scope: !2, file: !3, line: 2, type: !19, isLocal: true, isDefinition: true)
>> +!1 = distinct !DIGlobalVariable(name: "v1", scope: !2, file: !3, line: 2, type: !22, isLocal: true, isDefinition: true)
>> !2 = distinct !DICompileUnit(language: DW_LANG_C99, file: !3, producer: "clang version 8.0.20181009 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !4, retainedTypes: !5, globals: !7, nameTableKind: None)
>> !3 = !DIFile(filename: "test.c", directory: "/home/yhs/work/tests/llvm/bugs")
>> !4 = !{}
>> !5 = !{!6}
>> !6 = !DIBasicType(name: "long int", size: 64, encoding: DW_ATE_signed)
>> -!7 = !{!0, !8, !14}
>> +!7 = !{!0, !8, !14, !19}
>> !8 = !DIGlobalVariableExpression(var: !9, expr: !DIExpression())
>> !9 = distinct !DIGlobalVariable(name: "v2", scope: !2, file: !3, line: 3, type: !10, isLocal: true, isDefinition: true)
>> !10 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !11)
>> @@ -143,21 +166,25 @@ attributes #0 = { norecurse nounwind "co
>> !16 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !17)
>> !17 = !DIDerivedType(tag: DW_TAG_typedef, name: "int_ptr", file: !3, line: 1, baseType: !18)
>> !18 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !13, size: 64)
>> -!19 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !18)
>> -!20 = !{i32 2, !"Dwarf Version", i32 4}
>> -!21 = !{i32 2, !"Debug Info Version", i32 3}
>> -!22 = !{i32 1, !"wchar_size", i32 4}
>> -!23 = !{!"clang version 8.0.20181009 "}
>> -!24 = distinct !DISubprogram(name: "foo", scope: !3, file: !3, line: 5, type: !25, isLocal: false, isDefinition: true, scopeLine: 5, isOptimized: true, unit: !2, retainedNodes: !4)
>> -!25 = !DISubroutineType(types: !5)
>> -!26 = !DILocation(line: 5, column: 28, scope: !24)
>> -!27 = !{!28, !28, i64 0}
>> -!28 = !{!"any pointer", !29, i64 0}
>> -!29 = !{!"omnipotent char", !30, i64 0}
>> -!30 = !{!"Simple C/C++ TBAA"}
>> -!31 = !DILocation(line: 5, column: 33, scope: !24)
>> -!32 = !DILocation(line: 5, column: 31, scope: !24)
>> -!33 = !DILocation(line: 5, column: 38, scope: !24)
>> -!34 = !DILocation(line: 5, column: 36, scope: !24)
>> -!35 = !DILocation(line: 5, column: 21, scope: !24)
>> -!36 = !DILocation(line: 5, column: 14, scope: !24)
>> +!19 = !DIGlobalVariableExpression(var: !20, expr: !DIExpression())
>> +!20 = distinct !DIGlobalVariable(name: "v4", scope: !2, file: !3, line: 5, type: !21, isLocal: true, isDefinition: true)
>> +!21 = !DIDerivedType(tag: DW_TAG_const_type, baseType: !16)
>> +!22 = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: !18)
>> +!23 = !{i32 2, !"Dwarf Version", i32 4}
>> +!24 = !{i32 2, !"Debug Info Version", i32 3}
>> +!25 = !{i32 1, !"wchar_size", i32 4}
>> +!26 = !{!"clang version 8.0.20181009 "}
>> +!27 = distinct !DISubprogram(name: "foo", scope: !3, file: !3, line: 6, type: !28, isLocal: false, isDefinition: true, scopeLine: 6, isOptimized: true, unit: !2, retainedNodes: !4)
>> +!28 = !DISubroutineType(types: !5)
>> +!29 = !DILocation(line: 6, column: 28, scope: !27)
>> +!30 = !{!31, !31, i64 0}
>> +!31 = !{!"any pointer", !32, i64 0}
>> +!32 = !{!"omnipotent char", !33, i64 0}
>> +!33 = !{!"Simple C/C++ TBAA"}
>> +!34 = !DILocation(line: 6, column: 33, scope: !27)
>> +!35 = !DILocation(line: 6, column: 31, scope: !27)
>> +!36 = !DILocation(line: 6, column: 38, scope: !27)
>> +!37 = !DILocation(line: 6, column: 36, scope: !27)
>> +!38 = !DILocation(line: 6, column: 43, scope: !27)
>> +!39 = !DILocation(line: 6, column: 41, scope: !27)
>> +!40 = !DILocation(line: 6, column: 14, scope: !27)
>>
> Roman.
>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=DwIFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=DA8e1B5r073vIqRrFz7MRA&m=9Ed_8fP5kiQopGkmnLK5sLqoEFP7juhZ1lNRaWQgYoU&s=DkC0fIDyxomSE0n0VPREjIZmj4NwZdV3FU6Iz9RBr9A&e=
More information about the llvm-commits
mailing list